What a pain in the ass. This changes the `feature_cltv` functional test
to use the `MiniWallet`, an internal Pythonic wallet that produces
"signatures" consisting of several OP_NOPs in a row, so that this test
can be used when the wallet is disabled.
This means that the functions in feature_cltv which add a CLTV check and
change the locktime/sequence number no longer need to re-sign when they
change the txid, so they don't...HOWEVER, our Elements-specific
`feature_block_v4` test calls these functions with a real transaction
and depended on the re-signing behavior.
After several attempts at min-effort min-diff fixes I landed on this,
which simply sets the locktime at transaction creation and avoids having
to re-sign at all.
Also, I had to change the number of NOPs in the fake wallet code to make
a vsize assertion pass, presumably because of differences in Bitcoin/Elements
transaction serialization.
This is part of the "UTXO Set Statistics" project in Core, which aims
to make things like "checking coin supply" faster. This stuff, and
related things in coinstats.cpp, have always been broken for Elements
in the sense that we consider only explicit outputs and we ignore
assets (so everything just gets added together to get a meaningless
total). It probably wouldn't be too hard to restrict this to only
consider policyAsset, but it's out of scope for a rebase IMO.
Also, I think this situation is fine .. I don't understand the motivation
for this or why Core is merging this when they refuse to merge an address
index .. but I guess we'll see if there are users who care about this data
and who care about it being meaningful on Elements.
Also, apologies for the big diff -- there were some mechanical changes
to deal with CT amounts, but most of the changes related to the difference
in how fees are accounted for. While I'm not thrilled with this PR, its
functional test is really good! So I think what I eventually came up with
is internally consistent.
There are a few layers of bullshit to this PR.
First, there is the fact that it adds a functional test gated on a new
config flag which is disabled by default, so it actually adds broken
code with no tests, waiting to ruin your day 520 PRs later when #21935
enables the broken test.
Second, the test appears to be superficially nonsensical because it
generates two transactions from different wallets and tries to compare
them for byte-for-byte equality, which doesn't make sense (at least)
because change outputs are randomly located...so something fishy is
going on.
Of course, in Elements the transactions are *not* equal half the time
because the outputs are permuted, which may have let me quickly figure
out the issue, except...
Third, there is a red herring of a bug where the two transactions have
slightly different feerates. This turns out to be caused by
CWallet::CalculateMaximumSignedTxSize using differently sized dummy
transactions depending on whether watchonly outputs are included (this
fact is conveniently disguised by #17211 slightly changing this logic;
this is an unmerged PR in Core that Elements has a backport of an old
version of). And the two wallets have different watchonly settings.
A sub-red-herring is the fact that this bug results in a discrepancy
of 0.25 vbytes, so it does not appear in Core but does appear in
Elements (there is a 3/16 probability that we should be so unlucky...
we are).
But this is all irrelevant, because...
Fourth, this test is actually super bullshit. The way it works is by
constructing a PSBT legitimately, saving this to disk, then re-"signs"
using the external signer interface by using a mock signer that
COMPLETELY REPLACES THE TRANSACTION UNDER CONSTRUCTION. So it doesn't
matter what the fee output looks like and it doesn't matter what the
order of the outputs. Core does not detect this malfeasance and
neither does Elements. For some reason, Core has a functional test
that explicitly checks that you can do this even though it is insane
and it is hard to think of non-malicious reasons to do it.
Fifth, while Elements fails to detect that its external signer is
actually changing the transaction out from under it, it DOES assume
that this won't happen. In CWallet::SignPSBT it blithely un-replaces
the transaction, which undermines the functional test.
Sixth, the original PR where this test was introduced has comments
locked, so anyone who spent six hours reverse-engineering this idiotic
broken test, and is still feeling charitable enough to discuss it with
the Core developors, can go pound sand.
Anyway, just disabled the broken test and move on with our lives.
This PR eliminates "strange regtest=0 behavior" in a test which had forced
us to disable the test for Elements. Can re-enable now :)
I also removed the `chain_in_args` parameter to `TestNode`, which Steven added
in https://github.com/ElementsProject/elements/pull/533 (which itself replaces
unconditionally adding chain={} on the command-line, which was added in #458).
These were added in the 0.17 rebase to deal with the job of starting bitcoind,
which then did not support the `chain=` command-line arg as well as elementsd,
which back then required this command-line arg.
This was causing some issues with the "check -acceptnonstdtxn doesn't work on
mainnet" test because it would add -chain=elementsregtest to the command-line
of a daemon that was supposed to be connecting to mainnet/liquidv1. It is
possible to override this behavior, but since 0.20+ versions of elementsd and
bitcoind have essentially the same support for chain= options, it seemed
cleaner to just eliminate the diff.
This uses a regtest-only RPC which checks that the chain is literally
"regtest". Since regtest is disabled in Elements (we use elementsregtest)
I weakened the check for this to just check that the chain name has
"regtest" somewhere in it. Hopefully this isn't too magical.
fa066f1b66 test: Run feature_cltv with MiniWallet (MarcoFalke)
fa5591d38c test: Hide tx rehash in helper (MarcoFalke)
fa5f938cfe test: Remove new_tx reference (MarcoFalke)
Pull request description:
Allows to run the test even with no wallet compiled in
ACKs for top commit:
theStack:
ACK fa066f1b66💽
Tree-SHA512: 3f659a178ba3ee0baffd70fddf8b8a68e5551d85626c7f254b234d7f75e6a16430a32a7952037db358b579f045b4d296b46156f72e5d226f3e80334dc635ca10
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d
jonatack:
Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
fa40eb5b6b test: Speed up mempool_spend_coinbase.py (MarcoFalke)
fa29382ab2 test: Fix test cache issue (MarcoFalke)
fa085b470a test: Create MiniWallet.create_self_transfer (MarcoFalke)
fa1bedb494 test: Add MiniWallet.sendrawtransaction (MarcoFalke)
Pull request description:
Locally the test will run 4 seconds faster with `--valgrind` (18s vs 14s)
ACKs for top commit:
mjdietzx:
crACK https://github.com/bitcoin/bitcoin/pull/21762/commits/fa40eb5b6bfd151912c58d61771f6a6528f44e67
Tree-SHA512: ecfb60dda5ca5d7e6367bb9c6210390d95ebf6396ce657728901d118b75bb90c98f9351df3b01004d00682234448d6c6a13338d12097f7dced2cf7f1bd84d924
