fad0ae6bb8 doc: Rename fuzz seed_dir to corpus_dir (MarcoFalke)
Pull request description:
The fuzz corpus directory might contain hand-crafted seeds, but generally it is a set of test inputs. See also https://github.com/google/fuzzing/blob/master/docs/glossary.md#corpus
ACKs for top commit:
practicalswift:
cr ACK fad0ae6bb8: patch looks correct and "why not?" :)
fanquake:
ACK fad0ae6bb8 - did not test
Tree-SHA512: 38c952feb07aeeeb038b3261a12c824fab9ce5153d75f0ecf6d3f43db4f50998eeb2b14b11b7155f529189c93783fa2c11c81059021a04398c43f3505b31a2d4
48a0319bab Add a test that selects too large if BnB is used (Andrew Chow)
3e69939b78 Fail if maximum weight is too large (Andrew Chow)
51e2cd322c Have CalculateMaximumSignedTxSize also compute tx weight (Andrew Chow)
Pull request description:
Currently the `Transaction too large` is calculated on the transaction that is returned from `CreateTransaction`. This does not make sense for when `CreateTransaction` is being used for `fundrawtransaction` as no signing occurs so the final returned transaction is missing signatures. Thus users may successfully fund a transaction but fail to broadcast it after it has been fully signed.
So instead we should figure out whether the transaction we are funding will be too large after it is signed. We can do this by having `CalculateMaximumSignedTxSize` also return the transaction weight and then comparing that weight against the maximum weight.
ACKs for top commit:
instagibbs:
ACK 48a0319bab
meshcollider:
utACK 48a0319bab
Xekyo:
utACK with nits 48a0319bab
Tree-SHA512: 1700c60b07f67e2d5c591c5ccd131ac9f1861fab3def961c3c9c4b3281ec1063fe8e4f0f7f1038cac72692340856406bcee8fb45c8104d2ad34357a0ec878ac7
a061a29970 test: bring p2p_leak.py up to date. (Martin Zumsande)
Pull request description:
After the introduction of wtxidrelay and sendaddrv2 messages during version handshake, extend p2p_leak.py test to reflect this.
Also, some minor fixes and doc improvements.
I also added a test that peers not completing the version handshake will be disconnected for timeout, as suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19723#issuecomment-699540294.
ACKs for top commit:
brunoerg:
Tested ACK a061a29970
theStack:
Tested ACK a061a29970
Tree-SHA512: 26c601491fa8710fc972d1b8f15da6b387a95b42bbfb629ec4c668769ad3824b6dd6a33d97363bca2171e403d8d1ce08abf3e5c9cab34f98d53e0109b1c7a0e5
6a0a6e7d05 Correction for VerifyTaprootCommitment comments (Russell O'Connor)
Pull request description:
According to BIP-341, 'p' is called the taproot *internal* key, not inner key.
ACKs for top commit:
sipa:
ACK 6a0a6e7d05
benthecarman:
ACK 6a0a6e7d05
theStack:
ACK 6a0a6e7d05
Tree-SHA512: 94f553476a8404bff4b2d5724a1a54c5f530b987a616cd00a3800095f245c06e3c7a9066c729976f32069a56029406859a70ba523151d333dc1ed874f242bce8
233a886b42 test: check that getblockfilter RPC fails without block filter index (Sebastian Falbesoner)
Pull request description:
If a node was started without compact block filter index (parameter `--blockfilterindex=0`), the `getblockfilter` RPC call should fail.
ACKs for top commit:
MarcoFalke:
review ACK 233a886b42
Tree-SHA512: c8824373fad7d1de2dcb43c1d9541d736b478235be243080d2b7479c2588eac0e5722337ec1307394b331e0002fbcabb368e4955c2dc98dd5fce76d8c089e8a1
After the introduction of wtxidrelay and sendaddrv2 messages during
version handshake, extend p2p_leak.py test to reflect this.
Also, some minor fixes and doc improvements.
a701fcf01f net: Do not skip the I2P network from GetNetworkNames() (Vasil Dimov)
0181e24439 net: recognize I2P from ParseNetwork() so that -onlynet=i2p works (Vasil Dimov)
b905363fa8 net: accept incoming I2P connections from CConnman (Vasil Dimov)
0635233a1e net: make outgoing I2P connections from CConnman (Vasil Dimov)
9559bd1404 net: add I2P to the reachability map (Vasil Dimov)
76c35c60f3 init: introduce I2P connectivity options (Vasil Dimov)
c22daa2ecf net: implement the necessary parts of the I2P SAM protocol (Vasil Dimov)
5bac7e45e1 net: extend Sock with a method to check whether connected (Vasil Dimov)
42c779f503 net: extend Sock with methods for robust send & read until terminator (Vasil Dimov)
ea1845315a net: extend Sock::Wait() to report a timeout (Vasil Dimov)
78fdfbea66 net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions (Vasil Dimov)
34bcfab562 net: move the constant maxWait out of InterruptibleRecv() (Vasil Dimov)
cff65c4a27 net: extend CNetAddr::SetSpecial() to support I2P (Vasil Dimov)
f6c267db3b net: avoid unnecessary GetBindAddress() call (Vasil Dimov)
7c224fdac4 net: isolate the protocol-agnostic part of CConnman::AcceptConnection() (Vasil Dimov)
1f75a653dd net: get the bind address earlier in CConnman::AcceptConnection() (Vasil Dimov)
25605895af net: check for invalid socket earlier in CConnman::AcceptConnection() (Vasil Dimov)
545bc5f81d util: fix WriteBinaryFile() claiming success even if error occurred (Vasil Dimov)
8b6e4b3b23 util: fix ReadBinaryFile() returning partial contents (Vasil Dimov)
4cba2fdafa util: extract {Read,Write}BinaryFile() to its own files (Vasil Dimov)
Pull request description:
Add I2P support by using the [I2P SAM](https://geti2p.net/en/docs/api/samv3) protocol. Unlike Tor, for incoming connections we get the I2P address of the peer (and they also receive ours when we are the connection initiator).
Two new options are added:
```
-i2psam=<ip:port>
I2P SAM proxy to reach I2P peers and accept I2P connections (default:
none)
-i2pacceptincoming
If set and -i2psam is also set then incoming I2P connections are
accepted via the SAM proxy. If this is not set but -i2psam is set
then only outgoing connections will be made to the I2P network.
Ignored if -i2psam is not set. Notice that listening for incoming
I2P connections is done through the SAM proxy, not by binding to
a local address and port (default: true)
```
# Overview of the changes
## Make `ReadBinary()` and `WriteBinary()` reusable
We would need to dump the I2P private key to a file and read it back later. Move those two functions out of `torcontrol.cpp`.
```
util: extract {Read,Write}BinaryFile() to its own files
util: fix ReadBinaryFile() returning partial contents
util: fix WriteBinaryFile() claiming success even if error occurred
```
## Split `CConnman::AcceptConnection()`
Most of `CConnman::AcceptConnection()` is agnostic of how the socket was accepted. The other part of it deals with the details of the `accept(2)` system call. Split those so that the protocol-agnostic part can be reused if we accept a socket by other means.
```
net: check for invalid socket earlier in CConnman::AcceptConnection()
net: get the bind address earlier in CConnman::AcceptConnection()
net: isolate the protocol-agnostic part of CConnman::AcceptConnection()
net: avoid unnecessary GetBindAddress() call
```
## Implement the I2P [SAM](https://geti2p.net/en/docs/api/samv3) protocol (not all of it)
Just the parts that would enable us to make outgoing and accept incoming I2P connections.
```
net: extend CNetAddr::SetSpecial() to support I2P
net: move the constant maxWait out of InterruptibleRecv()
net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions
net: extend Sock::Wait() to report a timeout
net: extend Sock with methods for robust send & read until terminator
net: extend Sock with a method to check whether connected
net: implement the necessary parts of the I2P SAM protocol
```
## Use I2P SAM to connect to and accept connections from I2P peers
Profit from all of the preceding commits.
```
init: introduce I2P connectivity options
net: add I2P to the reachability map
net: make outgoing I2P connections from CConnman
net: accept incoming I2P connections from CConnman
net: recognize I2P from ParseNetwork() so that -onlynet=i2p works
net: Do not skip the I2P network from GetNetworkNames()
```
ACKs for top commit:
laanwj:
re-ACK a701fcf01f
jonatack:
re-ACK a701fcf01f reviewed diff per `git range-diff ad89812 2a7bb34 a701fcf`, debug built and launched bitcoind with i2pd v2.35 running a dual I2P+Torv3 service with the I2P config settings listed below (did not test `onlynet=i2p`); operation appears nominal (same as it has been these past weeks), and tested the bitcoind help outputs grepping for `-i i2p` and the rpc getpeerinfo and getnetworkinfo helps
Tree-SHA512: de42090c9c0bf23b43b5839f5b4fc4b3a2657bde1e45c796b5f3c7bf83cb8ec6ca4278f8a89e45108ece92f9b573cafea3b42a06bc09076b40a196c909b6610e
8a8c6383f6 zmq test: fix sync-up by matching notification to generated block (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR for #21008, fixes#21216.
In the course of investigating the problem with jnewbery (analyzing the Cirrus log https://cirrus-ci.com/task/4660108304056320), it turned out that the "sync up" procedure of repeatedly generating a block and waiting for a notification with timeout is too brittle in its current form, as the following scenario could happen:
- generate block A
- receive notification, timeout happens => repeat procedure
- generate block B
- node publishes block A notification
- receive notification, we receive the one caused by block A (!!!) => sync-up procedure is completed
- node publishes block B notification
- the actual test starts
- on the first notification reception, the one caused by block B is received, rather than the one actually caused by test code => assertion failure
This change in the PR ensures that after each test block generation, we wait for the notification that is actually caused by that block and ignore others from possibly earlier blocks. The matching is kind of ugly, it assumes that one out of four components in the block is contained in the notification: the block hash, the tx id, the raw block data or the raw transaction data. (Unfortunately we have to support all publisher topics.)
I'm aware that this is quite a lot of code now only for establishing a robust test setup. OTOH I wouldn't know of a better method right now, suggestions are very welcome.
Note for potential reviewers: for both reproducing the issue on master branch and verifying on PR branch, one can simply generate two blocks in the sync-up procedure rather than one.
ACKs for top commit:
MarcoFalke:
Concept ACK 8a8c6383f6
Tree-SHA512: a2eb78ba06dfd0fda7b1c111b6bbfb5dab4ab08500cc19c7ea02c3239495d5c74cc7d45250a8b3ecc78ca42d97ee6719bf73db8a137839e5e09a3cfcf08ed29e
So that help texts include "i2p" in:
* `./bitcoind -help` (in `-onlynet` description)
* `getpeerinfo` RPC
* `getnetworkinfo` RPC
Co-authored-by: Jon Atack <jon@atack.com>
Introduce two new options to reach the I2P network:
* `-i2psam=<ip:port>` point to the I2P SAM proxy. If this is set then
the I2P network is considered reachable and we can make outgoing
connections to I2P peers via that proxy. We listen for and accept
incoming connections from I2P peers if the below is set in addition to
`-i2psam=<ip:port>`
* `-i2pacceptincoming` if this is set together with `-i2psam=<ip:port>`
then we accept incoming I2P connections via the I2P SAM proxy.
It turned out that the "sync up" procedure of repeatedly generating a
block and waiting for a notification once with timeout is too naive in
its current form, as the following scenario could happen:
- generate block A
- receive notification, timeout happens -> repeat procedure
- generate block B
- node publishes block A notification
- receive notification, we receive the one caused by block A
-> sync-up procedure is completed
- node publishes block B
- the actual test starts
- on the first notification reception, one caused by block B is received,
rather than the one actually caused by test code, leading to failure
This change ensures that after each test block generation, we wait for
the notification that is actually caused by that block and ignore others
from possibly earlier blocks.
Co-authored-by: Jon Atack <jon@atack.com>
88c4b9b761 test: remove unneeded node from feature_blockfilterindex_prune.py (Jon Atack)
ace3f4cbdf test: improve assertions in feature_blockfilterindex_prune.py (Jon Atack)
Pull request description:
- improves the assertions
- removes an unneeded node, reducing from two to one, and some unneeded `extra_arg` code
ACKs for top commit:
MarcoFalke:
ACK 88c4b9b761
brunoerg:
Tested ACK 88c4b9b761
Tree-SHA512: 295700da3a5f583ee02ae2d184db93cce0e13aba69115d5db07f83e96a66b4b850adaff2c3725b6585799565b9ee654b1fee8a6245eaba8c21e1cb5ce524eb2b
Collects all the orphan handling globals into a single member var in
net_processing, and ensures access is encapuslated into the interface
functions. Also adds doxygen comments for methods.
fa730e9157 test: Avoid connecting to real network when running tests (MarcoFalke)
fa1b713941 test: Assume node is running in subtests (MarcoFalke)
Pull request description:
Introduced in #19884
ACKs for top commit:
Sjors:
ACK fa730e9157
Tree-SHA512: fe132a9ffe2fae1ab16857a3dec9839526fdf74d27a1ae794fbffca8356f639c4b916dc888b260281e9cc793916706c18d1687ebb5a076d4e1c481d218d308d3
fa560cc6c4 test: Intermittent issue in feature_blockfilterindex_prune (MarcoFalke)
Pull request description:
https://cirrus-ci.com/task/4962244553342976?command=ci#L5131
The index is built in a background thread, so we have to wait for it.
ACKs for top commit:
jonatack:
ACK fa560cc6c4
Tree-SHA512: e7a246fe43a28511581fe34b1f5a85303b1874b2535330afc0405269cce7306984ecc6af389791321e3aa4b224819e89d9b89dd5bc080d60baa20bd007412787
faa137eb9e test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c75 test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5 test: Remove unused bug workaround (MarcoFalke)
faabce7d07 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9e
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
ba7e17e073 rpc, test: document {previous,next}blockhash as optional (Sebastian Falbesoner)
Pull request description:
This PR updates the result help of the following RPCs w.r.t. the `previousblockhash` and `nextblockhash` fields:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain "previousblockhash") and best block (should not contain "nextblockhash").
Top commit has no ACKs.
Tree-SHA512: ef42c5c773fc436e1b4a67be14e2532e800e1e30e45e54a57431c6abb714d2c069c70d40ea4012d549293b823a1973b3f569484b3273679683b28ed40abf46bb
This option replaces --with-boost-process
This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.
This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini
Every (sub)test in the framework assumes the node is running, except for
the (sub)tests in this file. Remove that confusion by stopping the node
at the start of every subtest, instead of at the end.
fa24247d0f test: Fix NODE_NETWORK_LIMITED_MIN_BLOCKS disconnection (MarcoFalke)
fab6995629 test: Make test actually test something (MarcoFalke)
fae8f35df8 test: pep8 touched test (MarcoFalke)
Pull request description:
Fix several bugs. Also, fix#21227
ACKs for top commit:
jonasschnelli:
utACK fa24247d0f - thanks for fixing.
ryanofsky:
Code review ACK fa24247d0f with caveat above that I don't really understand the problem or fix. But the cleanups look good and the fix does seem perfectly safe. More description would be welcome!
Tree-SHA512: 67f6ec92f6493aa822ae3fa8a7426a5acdc684044b8bafc0c65b652f63ccce969d0a6f1d1f099d6a91d05f478724869345b70335f2cfcfd00df46aef05cc4f9e
6bfbc97d71 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019e76 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)
Pull request description:
Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.
Fixes#21104
ACKs for top commit:
jonatack:
ACK 6bfbc97d71
meshcollider:
utACK 6bfbc97d71
kristapsk:
ACK 6bfbc97d71. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.
Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
b4511e2e2e log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2e
MarcoFalke:
review ACK b4511e2e2e🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
9f21ed4037 [test] Check user agent string from test framework connections (John Newbery)
9ce4c3c4c1 [test] Add P2P_SERVICES to p2p.py (John Newbery)
010542614d [test] Move MY_RELAY to p2p.py (John Newbery)
9b4054cb7a [test] Move MY_SUBVERSION to p2p.py (John Newbery)
7e158a6910 [test] Move MY_VERSION to p2p.py (John Newbery)
652311165c [test] Move MIN_VERSION_SUPPORTED to p2p.py (John Newbery)
Pull request description:
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. This PR moves test framework specific constants to p2p.py.
It also changes the SUBVERSION constant to be a string instead of a bytes object. That means that it needs to be explicitly converted to a bytes object to serialize into a version message. Failing to do so would cause an easy-to-spot bug. This should avoid silent failures like the one solved in #20522.
ACKs for top commit:
laanwj:
Code review ACK 9f21ed4037
Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf
de6b389d5d tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152 wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5d
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5d modulo a few minor comments
fjahr:
Code review ACK de6b389d5d
meshcollider:
Tested ACK de6b389d5d
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
84716b134e Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb9 Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c3 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff768 Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11 Allow blockfilter in conjunction with prune (Jonas Schnelli)
Pull request description:
Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
This PR allows running the blockfilterindex(es) in conjunction with pruning.
* Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
* manual block pruning is disabled during blockfilterindex sync
* auto-pruning is delayed during blockfilterindex sync
ToDos:
* [x] Functional tests
ACKs for top commit:
fjahr:
Code review ACK 84716b1
ryanofsky:
Code review ACK 84716b134e. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
Add a check that new connections from the test framework to the
node have the correct user agent string. This makes bugs easier
to detect if the user agent string ever changes.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore specify the nServices value in the calling code,
not in the messages.py module.
messages.py is for message and primitive data structures. Specifics
about the test framework's p2p implementation should be in p2p.py.
Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to
relay. In Bitcoin Core, this is referred to as fRelay, since it's a
bool, so this field has always been misnamed.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_SUBVERSION to p2p.py.
Also rename to P2P_SUBVERSION.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_VERSION to p2p.py.
Also rename to P2P_VERSION to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
Also always set the nVersion field in CBlockLocator to 0 and ignore the
field in deserialized messages. The field is not currently used for
anything in Bitcoin Core.
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py.
Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
de85af5cce test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
1afc0e4aa1 doc: remove potentially confusing ChainstateManager comment (James O'Beirne)
769a1ef9fd test: Add tests with maleated snapshot data (Fabian Jahr)
4d8de04f32 tests: add snapshot activation test (James O'Beirne)
31d225274f tests: add deterministic chain generation unittest fixture (James O'Beirne)
6606a4f8c6 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (James O'Beirne)
ad949ba449 txdb: don't reset during in-memory cache resize (James O'Beirne)
f6e2da5fb7 simplify ChainstateManager::SnapshotBlockhash() return semantics (James O'Beirne)
7a6c46b37e chainparams: add allowed assumeutxo values (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.
Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.
Aside from that and the snapshot activation logic, there are a few related changes:
- ~~allow caching the `nChainTx` value in the CCoinsViewDB; this is set during snapshot activation. Because we don't necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.~~
- break out `CreateUTXOSnapshot()` from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
- ...and a few other misc. changes that are solely related to unittests.
The move-onlyish commit is most easily reviewed with `--color-moved=zebra`.
ACKs for top commit:
fjahr:
Code review ACK 1afc0e4aa1
laanwj:
Code review ACK 1afc0e4aa1
Tree-SHA512: a4e4f0698f00a53ec298b5e8b7ef1c9fdf0185f95139d1b1f63cfdf6cbbd6d17b8c6e51bbf1de2e5f1a946bf49f8466232698ef55acce5a012c80b067da366ea
ef21fb7313 zmq test: speedup test by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
5c6546362d zmq test: fix flakiness by using more robust sync method (Sebastian Falbesoner)
8666033630 zmq test: accept arbitrary sequence start number in ZMQSubscriber (Sebastian Falbesoner)
6014d6e1b5 zmq test: dedup message reception handling in ZMQSubscriber (Sebastian Falbesoner)
Pull request description:
Fixes#20934 by using the "sync up" method described in https://github.com/bitcoin/bitcoin/issues/20538#issuecomment-738791868.
After improving robustness with this approach (commits 1-3), it turned out that there were still some fails, but those were unrelated to zmq: Out of 500 runs, 3 times `sync_mempool()` or `sync_blocks()` timed out, which can happen because the trickle relay time has no upper bound -- hence in rare cases, it takes longer than 60s. This is fixed by enabling immediate tx relay on node1 (commit 4), which as a nice side-effect also gives us a rough 2x speedup for the test.
For further details, also see the explanations in the commit messages.
There is no guarantee that the test is still not flaky, but it would help if potential reviewers would run the following script locally and report how many runs failed (feel free to do less than 1000 runs, as this takes quite a long if ran with `--valgrind`):
```
#!/bin/sh
OUTPUT_FILE=./zmq_results
echo ===== repeated zmq test ===== > $OUTPUT_FILE
for i in `seq 1000`; do
echo ------------------------
echo ----- test run $i -----
echo ------------------------
echo --- $i --- >> $OUTPUT_FILE
./test/functional/interface_zmq.py --valgrind
if [ $? -ne 0 ]; then
echo "FAILED. /o\\" >> $OUTPUT_FILE
else
echo "PASSED. \\o/" >> $OUTPUT_FILE
fi
done
echo Failed test runs:
grep FAILED $OUTPUT_FILE | wc -l
```
ACKs for top commit:
jonatack:
Light ACK ef21fb7313 with the caveat that I was unable to make the test fail with valgrind both here and on master, so I can't vouch that it actually fixes the CI flakiness. The test does run ~2x faster with this.
Tree-SHA512: 7a1e7592fbbd98e69e1e1294486b91253e589c72b3c6bbb7f587028ec07cca59b7d984e4ebf256c4bc3e8a529ec77d31842f3dd874038aea0b684abfea50306a
96635e6177 init: use GetNetworkNames() in -onlynet help (Jon Atack)
0dbde700a6 rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps (Jon Atack)
1c3af37881 net: create GetNetworkNames() (Jon Atack)
b45eae4d53 net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName() (Jon Atack)
Pull request description:
per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87
- return a more helpful string name for `Network::NET_UNROUTABLE`: "not_publicly_routable" instead of "unroutable"
- update the RPC getpeerinfo "network" help, and automate it and the getnetworkinfo "network#name" and the -onlynet help doc generation
ACKs for top commit:
theStack:
re-ACK 96635e6177🌳
MarcoFalke:
review ACK 96635e6177🐗
Tree-SHA512: 511a7d987126b48a7a090739aa7c4964b6186a3ff8f5f7eec9233dd816c6b7a6dc91b3ea6b824aa68f218a8a3ebdc6ffd214e9a88af38f2bf23f3257c4284c3a
3f8776a139 Re-add dead code detection (flack)
Pull request description:
This re-adds unreachable code detection for Python based on `vulture`.
Effectively, this reverts f4beb4996d. The difference to the previous version is that this runs with the `--min-confidence 100` setting. From https://pypi.org/project/vulture/:
> Use `--min-confidence 100` to only report code that is guaranteed to be unused within the analyzed files.
So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a `return` statement). As such, there is not suppressions list.
My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place
ACKs for top commit:
practicalswift:
ACK 3f8776a139
Tree-SHA512: 52314ad4f627d969de1eb15375ca677ed86a2e816fe773756a1ce22421214ba407b5a09a4bf701a3aab1a10c7b336f548e4cef3327edf154acba55e987db21f6
f64adc1eed test: remove unused function xor_bytes (Sebastian Falbesoner)
Pull request description:
The function `xor_bytes` was introduced in commit 3c226639eb (#19953, BIP340-342 validation), even [code-reviewed](https://github.com/bitcoin/bitcoin/pull/19953/files#r509383731), but actually never used. The [default signing algorithm in BIP340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#Default_Signing) needs a xor operation, but this step is currently done by a single xor operation on large integer operands:
```
t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
```
Alternatively, we could keep the function and as well use it:
```diff
--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False):
P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)]))
if SECP256K1.has_even_y(P) == flip_p:
sec = SECP256K1_ORDER - sec
- t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
+ t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux))
kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER
assert kp != 0
R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)]))
```
ACKs for top commit:
practicalswift:
cr ACK f64adc1eed: untested unused code should be removed
Tree-SHA512: e9afae303488f19c6f6f44874d3537ed1c8164a197490e2b4e34853db886b858825b719648fa1a30b95177dcee9cf241f94ee9b835f0a2cae07024ce38a8c090
fe3e993968 [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta)
Pull request description:
Closes#19795
Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.
To reproduce:
`rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code
Other changes in the PR:
- `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds.
ACKs for top commit:
LarryRuane:
re-ACK fe3e993968
laanwj:
re-ACK fe3e993968
Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f
060a2a64d4 ci: remove boost thread installation (fanquake)
06e1d7d81d build: don't build or use Boost Thread (fanquake)
7097add83c refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef8 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d4
vasild:
ACK 060a2a64d4
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
e987ae5a55 test: Add test for deterministic UTXO set hash results (Fabian Jahr)
6ccc8fc067 test: Add test for gettxoutsetinfo RPC with MuHash (Fabian Jahr)
0d3b2f643d rpc: Add hash_type MUHASH to gettxoutsetinfo (Fabian Jahr)
2474645f3b refactor: Separate hash and stats calculation in coinstats (Fabian Jahr)
a1fcceac69 refactor: Improve encapsulation between MuHash3072 and Num3072 (Fabian Jahr)
Pull request description:
This is another Pr in the series PRs for Coinstatsindex (see overview in #18000). This PR adds the `hash_type` option `muhash` to `gettxoutsetinfo` through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet.
ACKs for top commit:
Sjors:
tACK e987ae5
achow101:
ACK e987ae5a55
jonatack:
Tested re-ACK e987ae5a55 per `git diff 3506d90 e987ae5`, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned `hash_serialized_2` of `2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454` and this branch returned `muhash` of `c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c`
ryanofsky:
Code review ACK e987ae5a55. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review.
Tree-SHA512: 9a739ce375e73749fa69a467262b60d3e5314ef384e2d7150b3bbc8e4125cd9fd1db95306623bb9a632fcbaf5d9d2bf2f5cc43bf717d4ff5e2c9c4b52dd9296c
3ddbf22ed1 util: Disallow negative mocktime (MarcoFalke)
f5f2f97168 net: Avoid UBSan warning in ProcessMessage(...) (practicalswift)
Pull request description:
Avoid UBSan warning in `ProcessMessage(...)`.
Context: https://github.com/bitcoin/bitcoin/pull/20380#issuecomment-770427182 (thanks Crypt-iQ!)
ACKs for top commit:
MarcoFalke:
re-ACK 3ddbf22ed1 only change is adding patch written by me
ajtowns:
ACK 3ddbf22ed1 -- code review only
Tree-SHA512: e8d7af0457ca86872b75a4e406c0a93aafd841c2962e244e147e748cc7ca118c56be0fdafe53765f4b291410030b2c3cc8f76f733b37a955d34fc885ab6037b9
fabeb5b9c7 fuzz: Disable shuffle when merge=1 (MarcoFalke)
Pull request description:
This should hopefully help make the deletion of fuzz inputs more deterministic.
My tests (N=1) revealed that without this patch 7000 files differ (https://github.com/bitcoin-core/qa-assets/pull/44#issuecomment-768841467). With this patch, "only" 2000 files differ.
ACKs for top commit:
practicalswift:
cr ACK fabeb5b9c7: `-shuffle=0` and `-prefer_small=1` make sense
Tree-SHA512: 21a701f52450d402a91dd6e0b33d564c63a9c3b919738eb9a80c24d48fc5b964088e325470738f39af0d595612c844acc7bf0941590cc2dc8c6f6ee4cb69c861
Speeds up the zmq test roughly by a factor of 2x (~20 sec. instead of
~40 sec.) and also avoids timeouts on the synchronization methods
(sync_mempool() / sync_blocks()) that happened with a slight chance.
This is due to the fact that there is no upper bound on the trickle
relay time, so even the default of 60s is sometimes too low. Fixed by
enabling immediate tx relay on node1.
After connecting the subscriber sockets to the node, there is no
guarantee that the node's zmq publisher interfaces are ready yet, which
means that potentially the first expected notification messages could
get lost and the test fails. Currently this is handled by just waiting
for a short period of time (200ms), which works most of the time but is
still problematic, as in some rare cases the setup time takes much
longer, even in the range of multiple seconds.
The solution in this commit approaches the problem by using a more
robust method of syncing up, originally proposed by instagibbs:
1. Generate a block on the node
2. Try to receive a notification on all subscribers
3. If all subscribers get a message within the timeout (1 second),
we are done, otherwise repeat starting from step 1
The ZMQSubscriber reception methods currently assert that the first
received publisher message has a sequence number of zero. In order to
fix the current test flakiness via "syncing up" to nodes in the setup
phase, we have to cope with the situation that messages get lost and the
first actual received message has a sequence number larger than zero.
fa0a4d6c60 test: remove assert_blockchain_height (MarcoFalke)
Pull request description:
This simplifies the code and solves intermittent timeouts caused by commit 0d39b5848a.
E.g. https://cirrus-ci.com/task/5196092369272832?command=ci#L3126
```
test 2021-02-08T12:27:56.275000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 127, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 180, in run_test
self.assert_blockchain_height(self.nodes[0], 101)
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 92, in assert_blockchain_height
assert False, "blockchain too short after timeout: %d" % current_height
AssertionError: blockchain too short after timeout: 101
ACKs for top commit:
glozow:
ACK fa0a4d6c60
Tree-SHA512: 3859b0c1581c21f03c775f119204cc3a219e5d86346883634886f6da46feaf254b9c6c49c1ec4581ffe409cdf05f6e91ec38c3976c3daf4a9e67f96ddc1e0dce
fa362064e3 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e3
glozow:
ACK fa362064e3🧸
jnewbery:
ACK fa362064e3
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
9913419cc9 test: remove type: comments in favour of actual annotations (fanquake)
Pull request description:
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than `# type:` comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
ACKs for top commit:
MarcoFalke:
review ACK 9913419cc9
jnewbery:
Code review ACK 9913419cc9
Tree-SHA512: 63aba5eef6c1320578f66cf8a6d85ac9dbab9d30b0d21e6e966be8216e63606de12321320c2958c67933bf68d10f2e76e9c43928e5989614cea34dde4187aad8
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than # type: comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
5e0cd25e29 fix the unreachable code at feature_taproot (Bruno Garcia)
Pull request description:
This PR removes the unnecessary return statement at the beginning of the function that makes the rest of the function unreachable.
ACKs for top commit:
practicalswift:
cr ACK 5e0cd25e29: patch looks correct!
sipa:
ACK 5e0cd25e29.
theStack:
Tested ACK 5e0cd25e29🏔️
sanket1729:
tACK 5e0cd25e29. I noted this a while ago while fixing feature_taproot.py for elements. Verified that the extreme ranges of CScriptNum are correct and the overflow case for `CHECKSIGADD` works as intended. Adding 1 to 2^31 - 1 results in an overflow, but the interpreter puts a `vch` of corresponding to 2^31 on stack. Even though it cannot be converted to CscriptNum(restricted to 4 bytes), it's result can still be compared by OP_EQUAL.
Tree-SHA512: fff9be3be94f4b3f3ccf24bf588d96e84d14806f82692dccd31631b0e5c79a7575a96c308cb5a4f610ab02e2f854b899f374437c33ecf6d52055d333f2de9b27
49797c3ccf tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269 Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe4 Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0 Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dba Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0 Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d4 Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7e tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389 tests: Set descriptors default based on compilation (Andrew Chow)
Pull request description:
This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.
For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework's MiniWallet.
ACKs for top commit:
laanwj:
ACK 49797c3ccf
ryanofsky:
Code review ACK 49797c3ccf. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is https://github.com/bitcoin/bitcoin/pull/20267#pullrequestreview-581508843
Tree-SHA512: 69659f8a81fb437ecbca962f4082c12835282dbf1fba7d9952f727a49e01981d749af9b09feda1c8ca737516c7d7a08ef17e782795df3fa69892d5021b41c1ed
