b01cd9471f test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc1981474 test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c48 test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
f8462a6d27/test/functional/feature_cltv.py (L26-L35)
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
9053b88b1c update docstring in feature_csv_activation.py (Pierre K)
Pull request description:
These changes in the test documentation reflect the changes introduced in #17921.
ACKs for top commit:
MarcoFalke:
review ACK 9053b88
Tree-SHA512: 17fb954baded8dab1c869dd48b76b516150bae616c792c573e4114d4adfdd40195745c56570aa3050cc0015ee496acd7ec178df8ba14831dd22f9722fda84da2
ffe33dfbd4 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2 versionbits: simplify state transitions (Anthony Towns)
55ac5f568a versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48 fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c tests: test versionbits delayed activation (Anthony Towns)
73d4a70639 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa tests: clean up versionbits test (Anthony Towns)
5932744450 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a47 tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK ffe33dfbd4
jnewbery:
utACK ffe33dfbd4
MarcoFalke:
review ACK ffe33dfbd4💈
achow101:
re-ACK ffe33dfbd4
gmaxwell:
ACK ffe33dfbd4
benthecarman:
ACK ffe33dfbd4
Sjors:
ACK ffe33dfbd4
jonatack:
Initial approach ACK ffe33dfbd4 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
41f891da50 tests: Skip SQLite fsyncs while testing (Andrew Chow)
Pull request description:
Since we want tests to run quickly, and since tests do a lot more db operations than expected we expect to see in actual usage, we disable sqlite's syncing behavior to make db operations run much faster. This syncing behavior is necessary for normal operation as it helps guarantee that data won't become lost or corrupted, but in tests, we don't care about that.
Fixes#21628
ACKs for top commit:
vasild:
ACK 41f891da50
Tree-SHA512: f36f969a182c622691ae5113573a3250e8d367437e83a1a9d3d2b55dd3a9cdf3c6474169a7bd271007bb9ce47f585aa7a6aeae6eebbaeb02d79409b02f47fd8b
Since we want tests to run quickly, and since tests do a lot more db
operations than expected we expect to see in actual usage, we disable
sqlite's syncing behavior to make db operations run much faster. This
syncing behavior is necessary for normal operation as it helps guarantee
that data won't become lost or corrupted, but in tests, we don't care
about that.
This commit moves the ExternalSigner class and RPC methods out of the wallet module.
The enumeratesigners RPC can be used without a wallet since #21417.
With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.
The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context.
A future displayaddress RPC call without wallet context could take a descriptor argument.
This commit fixes a rpc_help.py failure when configured with --disable-wallet.
5c446784b1 rpc: improve getnodeaddresses help (Jon Atack)
1b9189866a rpc: simplify/constify getnodeaddresses code (Jon Atack)
3bb6e7b655 rpc: add network field to rpc getnodeaddresses (Jon Atack)
Pull request description:
This patch adds a network field to RPC `getnodeaddresses`, which is useful on its own, particularly with the addition of new networks like I2P and others in the future, and which I also found helpful for adding a new CLI command as a follow-up to this pull that calls `getnodeaddresses` and needs to know the network of each address.
While here, also improve the `getnodeaddresses` code and help.
```
$ bitcoin-cli -signet getnodeaddresses 3
[
{
"time": 1611564659,
"services": 1033,
"address": "2600:1702:3c30:734f:8f2e:744b:2a51:dfa5",
"port": 38333,
"network": "ipv6"
},
{
"time": 1617531931,
"services": 1033,
"address": "153.126.143.201",
"port": 38333,
"network": "ipv4"
},
{
"time": 1617473058,
"services": 1033,
"address": "nsgyo7begau4yecc46ljfecaykyzszcseapxmtu6adrfagfrrzrlngyd.onion",
"port": 38333,
"network": "onion"
}
]
$ bitcoin-cli help getnodeaddresses
getnodeaddresses ( count )
Return known addresses, which can potentially be used to find new nodes in the network.
Arguments:
1. count (numeric, optional, default=1) The maximum number of addresses to return. Specify 0 to return all known addresses.
Result:
[ (json array)
{ (json object)
"time" : xxx, (numeric) The UNIX epoch time when the node was last seen
"services" : n, (numeric) The services offered by the node
"address" : "str", (string) The address of the node
"port" : n, (numeric) The port number of the node
"network" : "str" (string) The network (ipv4, ipv6, onion, i2p) the node connected through
},
...
]
```
Future idea: allow passing `getnodeaddresses` a network (or networks) as an argument to return only addresses in that network.
ACKs for top commit:
laanwj:
Tested ACK 5c446784b1
jarolrod:
re-ACK 5c446784b1
promag:
Code review ACK 5c446784b1.
Tree-SHA512: ab0101f50c76d98c3204133b9f2ab6b7b17193ada31455ef706ad11afbf48f472fa3deb33e96028682369b35710ccd07d81863d2fd55c1485f32432f2b75efa8
a97a9298ce Test that signrawtx works when a signed CSV and CLTV inputs are present (Andrew Chow)
6965456c10 Introduce DeferringSignatureChecker and inherit with SignatureExtractor (Andrew Chow)
Pull request description:
Previously SignatureExtractorChecker took a MutableTransactionSignatureChecker and passed through function calls to that. However not all functions were implemented so not everything passed through as it should have. To solve this, SignatureExctractorChecker now implements all of those functions via a new class - DeferredSignatureChecker. DeferredSignatureChecker is introduced to allow for future signature checkers which use another SignatureChecker but need to be able to do somethings outside of just the signature checking.
Fixes#21151
ACKs for top commit:
sipa:
utACK a97a9298ce
meshcollider:
Code review ACK a97a9298ce
instagibbs:
utACK a97a9298ce
Tree-SHA512: bca784c75c2fc3fcb74e81f4e3ff516699e8debaa2db81e12843abdfe9cf265dac11db8619751cb9b3e9bbe779805d029fabe5f3cbca5e86bfd72de3664b0b94
2e5f7def22 wallet, rpc: update listdescriptors response format (Ivan Metlushko)
Pull request description:
Update `listdescriptors` response format according to [RPC interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines).
This is a follow up for #20226
**Before:**
```
Result:
[ (json array) Response is an array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
```
**After:**
```
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"descriptors" : [ (json array) Array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
}
```
ACKs for top commit:
achow101:
re-ACK 2e5f7def22
meshcollider:
utACK 2e5f7def22
jonatack:
re-ACK 2e5f7def22
Tree-SHA512: 49bf73e46e2a61003ce594a4bfc506eb9592ccb799c2909c43a1a527490a4b4009f78dc09f3d47b4e945d3d7bb3cd2632cf48c5ace5feed5066158cc010dddc1
ccd976dd3d test: use 327 fewer blocks in feature_nulldummy (Jon Atack)
68c280f197 test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack)
Pull request description:
The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug`
Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion.
ACKs for top commit:
AnthonyRonning:
reACK ccd976dd3d - ran a few times with the rest of the tests and still passing for me with just the fewer block change.
MarcoFalke:
review ACK ccd976dd3d 🏝
Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
90ae3d8ca6 doc: Add release notes for -deprecatedrpc=addresses and bitcoin-tx (Michael Dietz)
085b3a7299 rpc: deprecate `addresses` and `reqSigs` from rpc outputs (Michael Dietz)
Pull request description:
Considering the limited applicability of `reqSigs` and the confusing output of `1` in all cases except bare multisig, the `addresses` and `reqSigs` outputs are removed for all rpc commands.
1) add a new sane "address" field (for outputs that have an identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact (with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely always.
Note: Some light refactoring done to allow us to very easily delete a few chunks of code (marked with TODOs) when we remove this deprecated behavior.
Using `IsDeprecatedRPCEnabled` in core_write.cpp caused some circular dependencies involving core_io
Circular dependencies were caused by rpc/util unnecessarily importing node/coinstats and node/transaction. Really what rpc/util needs are some fundamental type/helper-function definitions. So this was cleaned up to make more sense.
This fixes#20102.
ACKs for top commit:
MarcoFalke:
re-ACK 90ae3d8ca6📢
Tree-SHA512: 8ffb617053b5f4a8b055da17c06711fd19632e0037d71c4c8135e50c8cd7a19163989484e4e0f17a6cc48bd597f04ecbfd609aef54b7d1d1e76a784214fcf72a
beead33a21 [test] no send feefilters when txrelay is turned off (glozow)
18a9b27dd6 p2p: Don't send FEEFILTER in blocksonly mode (Martin Zumsande)
Pull request description:
The purpose of FEEFILTER messages (BIP 133) is to inform our peers that we do not want transactions below a specified fee rate.
In blocksonly mode, we do not want our peer to send us any transactions at all (and will disconnect if a peer still sends a transaction INV or TX).
Therefore, I don't think that it makes sense to send FEEFILTER messages every 10 minutes on average in blocksonly mode - this PR disables it.
Note that on block-relay-only connections, FEEFILTER is already disabled, just not in blocksonly mode.
ACKs for top commit:
glozow:
re ACK beead33a21🙂 thanks for adding the test!
amitiuttarwar:
reACK beead33a21
MarcoFalke:
review ACK beead33a21
jnewbery:
reACK beead33a21
Tree-SHA512: e748cd52fe23d647fa49008b020389956ac508e16ce9fd108d8afb773bff95788298ae229162bd70215d7246fc25c796484966dc05890b0b4ef601f9cd35628b
4f2653a890 test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c5175c test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1d7b rpc: Improve gettxoutsetinfo help (Fabian Jahr)
Pull request description:
Follow-ups to #19145:
- Small improvement on the help text of RPC gettxoutsetinfo
- Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
- Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`
Split out of #19521.
ACKs for top commit:
MarcoFalke:
review ACK 4f2653a890👲
Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
39a9ec579f Unconditionally check for fRelay field in test framework (Troy Giorshev)
Pull request description:
picking up #20411 (rebased onto master)
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead, we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
ACKs for top commit:
jnewbery:
utACK 39a9ec579f
Tree-SHA512: 13a23f1180b7121ba41cb85baa38094b41f4607a7c88b3384775177cb116e76faf5514760624f98a4e8a830767407c46753a7e0285158c33e0c6ce395de8f15c
581791c620 test: add functional test for anchors.dat (bruno)
Pull request description:
This PR adds a functional test for anchors.dat.
It creates a node and adds 2 outbound block-relay-only connections and 5 inbound connections.
When the node is down, anchors.dat should contain the 2 addresses from the outbound block-relay-only connections.
ACKs for top commit:
MarcoFalke:
Concept ACK 581791c620
hebasto:
ACK 581791c620
Tree-SHA512: 77038b09e36ee5ae473a26d6f566c0ed283af258c34df8486706a24f72b05abab621a293ac886d03849bc45bc28be7336137252225b25aff393baa6b5238688c
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
8dd5946c0b add functional test (Larry Ruane)
b5a80fa7e4 util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)
Pull request description:
If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.
ACKs for top commit:
fjahr:
tACK 8dd5946c0b
luke-jr:
utACK 8dd5946c0b (no changes since previous utACK)
hebasto:
re-ACK 8dd5946c0b, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/18335#pullrequestreview-460621350) review.
darosior:
ACK 8dd5946c0b
Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
d09120b7d1 test: give fundraw more time for test_transaction_too_large (Jon Atack)
Pull request description:
to hopefully fix timeouts from a new test added in 48a0319bab of #20536 merged March 8, 2021
seen locally when running via the test runner
```
File "/home/jon/projects/bitcoin/bitcoin/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large
raise JSONRPCException({
test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
and in the CI like https://bitcoinbuilds.org/index.php?ansilog=28537952-2c92-46f2-9871-8918e5ba2738.log#l2398
```
File "/home/ubuntu/src/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large
test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 240.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
Top commit has no ACKs.
Tree-SHA512: f11c923439014fe12420f986c640fd301a26282eb41516957d73b9c751087cbae3d0e316f9ccb49bcb424f488540266f70d3f97948633e77c62bd7935df90452
c62f9bc0e9 test: use fewer blocks in wallet_groups and move sync call (Jon Atack)
3a16b5ef95 test: add missing logging to wallet_groups.py (Jon Atack)
Pull request description:
- add logging (particularly useful as the tests are somewhat slow)
- generate 101 blocks instead of 110
- move `sync_all` call into the loop, so fewer blocks are synced on each call, to hopefully see fewer CI timeouts as in https://bitcoinbuilds.org/index.php?ansilog=88eee99e-1727-44ed-b778-3b9c75c33928.log
```
L2742 File "/home/ubuntu/src/test/functional/wallet_groups.py", line 162, in run_test
L2743 self.sync_all()
test_framework.authproxy.JSONRPCException: 'syncwithvalidationinterfacequeue' RPC took longer than 960.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
ACKs for top commit:
MarcoFalke:
cr ACK c62f9bc0e9
Tree-SHA512: 711deafcd589cb8196cb207ff882e0f2ab6b70828a6abad91f83f535974cc430a56b9e8a960fd233d31d610932a0d48b49ee681aae564d145a3040288ecda8f8
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
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
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
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
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
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
590bda79e8 scripted-diff: Remove setup_clean_chain if default is not changed (Fabian Jahr)
98892f39e3 doc: Improve setup_clean_chain documentation (Fabian Jahr)
Pull request description:
The first commit improves documentation on setup_clean_chain which is misunderstood quite frequently. Most importantly it fixes the TestShell docs which are simply incorrect.
The second commit removes the instances of `setup_clean_clain` in functional tests where it is not changing the default.
This used to be part of #19168 which also sought to rename`setup_clean_chain`.
ACKs for top commit:
jonatack:
ACK 590bda79e8
Tree-SHA512: a7881186e65d31160b8f84107fb185973b37c6e50f190a85c6e2906a13a7472bb4efa9440bd37fe0a9ac5cd2d1e8559870a7e4380632d9a249eca8980b945f3e
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
