Test that passing conflicting parameters for the two causes a startup error.
This logic also impacts -connect, which soft sets -dnsseed, so add a test for
that too.
When a node is able to properly shutdown, it will persist its block-relay-only
connections to the addrman. On startup, it will attempt to reconnect to these
anchors. Since block-relay-only connections do not participate in ADDR relay,
succesful connections are insufficient to skip querying the DNS seeds.
This test fails prior to the changes in #22013.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This commit introduces a DNS seed to the regest chain params in order to add
coverage to the DNS querying logic.
The first test checks that we do not query DNS seeds if we are able to
succesfully connect to 2 outbound connections. Since we participate in ADDR
relay with those connections, including sending a GETADDR message during the
VERSION handshake, querying the DNS seeds is unnecessary.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
aa02c64540 test: use MiniWallet for simple doublespend test in feature_rbf.py (Sebastian Falbesoner)
a3f6397c73 test: feature_rbf.py: make MiniWallet instance available for all sub-tests (Sebastian Falbesoner)
84c874794c test: remove unneeded initialization code in feature_rbf.py (Sebastian Falbesoner)
Pull request description:
This PR's goal is to prepare the functional test `feature_rbf.py` for more MiniWallet usage. It first gets rid of unused initialization code (I guess that was needed at times when the nodes were still in IBD at the start of tests?), then makes the MiniWallet instance introduced in #22210 available for all sub-tests, and finally, uses that instance in the first sub-test `test_simple_doublespend`.
Note that the same idea of replacing the `make_utxo` calls with MiniWallet can be also applied to other sub-tests too; this just serves as a first proof-of-concept.
ACKs for top commit:
MarcoFalke:
re-ACK aa02c64540🌯
Tree-SHA512: 2902dd15d493d83b0790028c92d14fbd99ca05ace704c7011fb38261ce6517aeb810ef9f360fcb701d95887975b6a2911cfe538858d38fceb2c1c2a40afdbe6b
8ca51af1ec test: Disable automatic connections by default (Martin Zumsande)
Pull request description:
A node normally doesn't make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), `ThreadOpenConnections` will periodically try to connect to them, resulting in log entries such as:
`[opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs`
I don't think it's desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test.
This PR fixes this problem by disabling `ThreadOpenConnections` by adding `-connect=0` to the default args, and adding exceptions only when needed for the test to pass.
ACKs for top commit:
tryphe:
Concept ACK, light code review ACK 8ca51af1ec
Tree-SHA512: bcfb2de610e6c35a97a2bd7ad6267e968b1ac7529638d99276993cd5bc93ce9919d54e22d6dc84e1b02ecd626ab6554e201693552ea065c29794eece38c43f7d
ef5e9304cd test: update logging and docstring in rpc_blockchain.py (Jon Atack)
d548dc71e4 test: replace magic values by constants in rpc_blockchain.py (Jon Atack)
78c361086f test: assert on mediantime in getblockheader and getblockchaininfo (Jon Atack)
0a9129c588 test: assert on the value of getblockchaininfo#time (Jon Atack)
Pull request description:
Follow-up to #22407 improving test coverage per https://github.com/bitcoin/bitcoin/pull/22407#pullrequestreview-702077013.
ACKs for top commit:
tryphe:
untested ACK ef5e9304cd
Tree-SHA512: f746d56f430331bc6a2ea7ecd27b21b06275927966aacf1f1127d8d5fdfd930583cabe72e23df3adb2e005da904fc05dc573b8e5eaa2f86e0e193b89a17a5734
This test checks that we only relay addresses with inbound peers who have sent
us an addr related message. Uses a combination of GETADDR and ADDR to verify
when peers are eligible.
Use SetupAddressRelay to only initialize `m_addr_known` as needed. For outbound
peers, we initialize the filter before sending our self announcement (not
applicable for block-relay-only connections). For inbound peers, we initialize
the filter when we get an addr related message (ADDR, ADDRV2, GETADDR).
These changes intend to mitigate address blackholes. Since an inbound peer has
to send us an addr related message to become eligible as a candidate for addr
relay, this should reduce our likelihood of sending them self-announcements.
Use an init param to make clear whether a getaddr message should be sent when
the P2PConnection receives a version message. These changes are in preparation
for upcoming commits that modify the behavior of a bitcoind node and the test
framework.
fbeb8c43bc test: add type annotations to util.get_rpc_proxy (fanquake)
Pull request description:
Split out from #22092 while we address the functional test failure.
ACKs for top commit:
instagibbs:
ACK fbeb8c43bc
Tree-SHA512: 031ef8703202ae5271787719fc3fea8693574b2eb937ccf852875de95798d7fa3c39a8db7c91993d0c946b45d9b4d6de570bd1102e0344348784723bd84803a8
f685a13bef doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f08 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef
LarryRuane:
Code review, test ACK f685a13bef
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
779e638ca9 coinstats: Add comments for new coinstatsindex values (Fabian Jahr)
5b3d4e724f Index: Improve logging in coinstatsindex (Fabian Jahr)
d4356d4e48 rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo (Fabian Jahr)
a5f6791139 rpc: Add missing gettxoutsetinfo help docs (Fabian Jahr)
01386bfd88 Index: Return early from failed coinstatsindex init (Fabian Jahr)
1e3842385b index: Use batch writing in coinstatsindex WriteBlock (Fabian Jahr)
fb65dde147 scripted-diff: Fix coinstats data member names (Fabian Jahr)
8ea8c927ac index: Avoid unnecessary type casts in coinstatsindex (Fabian Jahr)
Pull request description:
This is a collection of smaller follow-ups to #19521, addressing several post-merge review comments.
ACKs for top commit:
Sjors:
re-utACK 779e638ca9
jonatack:
re-ACK 779e638ca9 diff since last review involves doc changes only; rebased to current master and verified clean debug build/no silent conflicts, unit tests, and feature_coinstatsindex functional test
laanwj:
Code review ACK 779e638ca9
Talkless:
re-utACK 779e638ca9 after cosmetic changes.
Tree-SHA512: cb0d038d230c582d7fe3041c89b1e04d39971fab3739d540c609cf826754c6c513b12ded08ac92180aec7a9d7a70114ece50357bd1a902de4adaae9f30b8d699
a006d7d730 test: add logging to wallet_listtransactions (Sebastian Falbesoner)
47915b1187 test: remove unneeded/redundant code in wallet_listtransactions (Sebastian Falbesoner)
fb6c6a7938 test: speedup wallet_listtransactions by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
This PR improves the test `wallet_listtransactions.py` in three ways:
* speeds up runtime by a factor of 2-3x by using the good ol' immediate tx relay trick (`-whitelist=noban@127.0.0.1`)
* removes unneeded/redundant code
* adds log messages, mostly by turning comments into `self.log.info(...)` calls
ACKs for top commit:
jonatack:
ACK a006d7d730
kristapsk:
ACK a006d7d730
Tree-SHA512: a91a19f5ebc4d05f0b96c5419683c4c57ac0ef44b64eeb8dd550bd72296fd3a2857a3ba83f755fe4b0b3bd06439973f226070b5d0ce2dee58344dae78cb50290
This prevents the node from trying to connect to random IPs on the internet
while running the functional tests. Exceptions are added when required for
the test to pass.
Without this check, the tests would also pass if the CSV and
CLTV activation heights are not reached yet (e.g. if the .generate()
calls before are removed), as the operations OP_CSV and OP_CLTV
simply behave as NOPs.
Also fixes a comment in the sub-test `test_signing_with_cltv()`.
During initial sync after startup the gettxoutsetinfo RPC will still return an error while catching up. However, after the initial sync the index will not error immediately anymore when it's in the process of syncing to the tip while being called. Instead it will block until synced and then return the response.
179a051704 util: improves error messages on get_previous_releases script (Nelson Galdeman)
Pull request description:
When previous releases are fetched and the specified version wasn't added to the checksum list we used to get a "Checksum did not match" which isn't true (https://github.com/bitcoin-core/bitcoincore.org/issues/753#issuecomment-879546719).
If the specified version number is not on the list, it now logs cannot do the comparison instead.
ACKs for top commit:
practicalswift:
cr ACK 179a051704
theStack:
tACK 179a051704, tested on Debian bullseye/sid
Tree-SHA512: 2a07ce75232f853fd311c43581f8faf12d423668946ae6ad784feece5b4d0edd57fc018ba1f0c5a73bfaccb326e0df9a643580d16bf427c1ec3ff34a9cdbc80c
a806647d26 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db7 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b82 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd64 [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)
Pull request description:
Builds on #21009 and makes progress on remaining items in #17862
Removing `RewindBlockIndex()` in #21009 allows the following:
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
mzumsande:
Code-Review ACK a806647d26
laanwj:
Code review ACK a806647d26, nice cleanup
jnewbery:
utACK a806647d26
theStack:
ACK a806647d26
Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e
2ebf2fe0e4 test: check for RPC error 'Transaction already in block chain' (-27) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the RPC error "Transaction already in block chain" (error code `RPC_VERIFY_ALREADY_IN_CHAIN` = `RPC_TRANSACTION_ALREADY_IN_CHAIN` = -27), which is thrown in the function `BroadcastTransaction` (src/node/transaction.cpp).
ACKs for top commit:
kristapsk:
ACK 2ebf2fe0e4 (ran linter, looked at changes and ran modified test and checked code in `src/node/transaction.cpp`)
darosior:
ACK 2ebf2fe0e4
Tree-SHA512: 8bfbd3ff3da0cb3b8745f69b8ca2377f85fa99f0270750840b60e6ae43b5645c5c59b236993e8b2ad0444ec4171484e4f1ee23fa7e81b79d4222bcb623666fa5
20edf4bcf6 rpc: Return block time in getblockchaininfo (João Barbosa)
Pull request description:
Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.
ACKs for top commit:
naumenkogs:
ACK 20edf4bcf6
theStack:
re-ACK 20edf4bcf6
0xB10C:
ACK 20edf4bcf6
kristapsk:
ACK 20edf4bcf6
Zero-1729:
re-ACK 20edf4bcf6
Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
9b85a5e2f7 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7🎰
ryanofsky:
Code review ACK 9b85a5e2f7. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
5a77abd4e6 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c036 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1ea [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)
Pull request description:
1. Only add a transaction to the unbroadcast set when it's added to the mempool
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added to the mempool.
2. Allow rebroadcast for same-txid-different-wtxid transactions
There is some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.
ACKs for top commit:
duncandean:
reACK 5a77abd
naumenkogs:
ACK 5a77abd4e6
theStack:
re-ACK 5a77abd4e6
lsilva01:
re-ACK 5a77abd4e6
Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
fac4814106 doc/release-process: Add torrent creation details (Carl Dong)
5d24cc3d82 guix/INSTALL: Guix installs init scripts in libdir (Carl Dong)
5da2ee49d5 guix/INSTALL: Add coreutils/inotify-dir-recreate troubleshooting (Carl Dong)
318c60700b guix: Adapt release-process.md to new Guix process (Carl Dong)
fcab35b229 guix-attest: Produce and sign normalized documents (Carl Dong)
c2541fd0ca guix: Overhaul README (Carl Dong)
46ce6ce378 tree-wide: Rename gitian-keys to builder-keys (Carl Dong)
fc4f8449f3 guix: Update various check_tools lists (Carl Dong)
263220a85c guix: Check for a sane services database (Carl Dong)
Pull request description:
Based on: #21462
Keeping the README in one file so that it's easy to search through. Will add more jumping links later so navigation is easier.
Current TODOs:
- [x] Shell installer option: prompt user to re-login for `/etc/profile.d` entry to be picked up
- [x] Binary tarball option: prompt user to create `/etc/profile.d` entry and re-login
- [x] Fanquake docker option: complete section
- [x] Arch Linux AUR option: prompt to start `guix-daemon-latest` unit after finishing "optional setup" section
- [x] Building from source option: Insert dependency tree diagram that I made
- [x] Building from source option: redo sectioning, kind of a mess right now
- [x] Optional setup: make clear which parts are only needed if building from source
- [x] Workaround 1 for GnuTLS: perhaps mention how to remove Guix build farm's key
- [x] Overall (after everything): Make the links work.
Note to self: wherever possible, tell user how to check that something is true rather than branching by installation option.
ACKs for top commit:
fanquake:
ACK fac4814106 - going to go ahead and merge this now. It's a lot of documentation, and could probably be nit-picked / improved further, however, that can continue over the next few weeks. I'm sure more (backportable) improvements / clarifications will be made while we progress through RCs towards a new release.
Tree-SHA512: dc46c0ecdfc67c7c7743ca26e4a603eb3f54adbf81be2f4c1f4c20577ebb84b5250b9c9ec89c0e9860337ab1c7cff94d7963c603287267deecfe1cd987fa070a
Adds a test for the condition which can trigger a lock order assertion.
Specifically, there must be an unconfirmed transaction in the mempool
which belongs to the wallet being loaded. This will establish the order
of cs_wallet -> cs_main -> cs_KeyStore. Then dumpwallet is called on
that wallet. Previously, this would have used a lock order of cs_wallet
-> cs_KeyStore -> cs_main, but this should be fixed now. The test
ensures that.
a4bcd687c9 Improve tests using statistics (John Newbery)
f424d601e1 Add logging and addr rate limiting statistics (Pieter Wuille)
b4ece8a1cd Functional tests for addr rate limiting (Pieter Wuille)
5648138f59 Randomize the order of addr processing (Pieter Wuille)
0d64b8f709 Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
ACKs for top commit:
laanwj:
ACK a4bcd687c9
vasild:
ACK a4bcd687c9
jnewbery:
ACK a4bcd687c9
jonatack:
ACK a4bcd687c9
Tree-SHA512: b757de76ad78a53035b622944c4213b29b3b55d3d98bf23585afa84bfba10808299d858649f92269a16abfa75eb4366ea047eae3216f7e2f6d3c455782a16bea
-> remove unneeded get-out-of IBD generate()
(The test framework already sets up the nodes to be out of IBD
in setup_nodes(), if setup_clean_chain is not set to True)
-> remove duplicate code line assigning an utxo
By whitelisting the peers via -whitelist, the inventory is transmissioned
immediately rather than on average every 5 seconds, speeding up the test by at
least a factor of two:
before:
$ time ./wallet_listtransactions.py
...
0m40.25s real 0m01.74s user 0m01.70s system
with this PR:
$ time ./wallet_listtransactions.py
...
0m14.93s real 0m01.68s user 0m01.87s system
This commit also moves the wallet_listtransactions tests into the < 30s group.
While limitations on the influence of attackers on addrman already
exist (affected buckets are restricted to a subset based on incoming
IP / network group), there is no reason to permit them to let them
feed us addresses at more than a multiple of the normal network
rate.
This commit introduces a "token bucket" rate limiter for the
processing of addresses in incoming ADDR and ADDRV2 messages.
Every connection gets an associated token bucket. Processing an
address in an ADDR or ADDRV2 message from non-whitelisted peers
consumes a token from the bucket. If the bucket is empty, the
address is ignored (it is not forwarded or processed). The token
counter increases at a rate of 0.1 tokens per second, and will
accrue up to a maximum of 1000 tokens (the maximum we accept in a
single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
immediately gets 1000 additional tokens, as we actively desire many
addresses from such peers (this may temporarily cause the token
count to exceed 1000).
The rate limit of 0.1 addr/s was chosen based on observation of
honest nodes on the network. Activity in general from most nodes
is either 0, or up to a maximum around 0.025 addr/s for recent
Bitcoin Core nodes. A few (self-identified, through subver) crawler
nodes occasionally exceed 0.1 addr/s.
7593b06bd1 test: ensure I2P addresses are relayed (Vasil Dimov)
e7468139a1 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d2a4 test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
86742811ce test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02708 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)
Pull request description:
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.
Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
ACKs for top commit:
jonatack:
ACK 7593b06bd1
naumenkogs:
ACK 7593b06bd1.
laanwj:
Code review ACK 7593b06bd1
kristapsk:
ACK 7593b06bd1. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.
Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
a3d6ec5bb5 test: move rpc_rawtransaction tests to < 30s group (Jon Atack)
5a1ed96077 test: whitelist rpc_rawtransaction peers to speed up tests (Jon Atack)
Pull request description:
Speed up the somewhat slow `rpc_rawtransaction.py` test by more than 3x (from 45-55 seconds to 15 seconds on a laptop running 2 x 2.5GHz).
ACKs for top commit:
mjdietzx:
ACK a3d6ec5bb5
kristapsk:
ACK a3d6ec5bb5
theStack:
ACK a3d6ec5bb5🐎
brunoerg:
tACK a3d6ec5bb5
Tree-SHA512: f1d105594c9b5b257a7096b631a6fa5aeb50e330a351f75c2d6ffa7dd73abdb6e1f596a78c16d204a9bac3fe506e0519f9ad96bb8477ab6424c8e18125ccb659
fa80e10d94 test: Add feature_taproot.py --previous_release (MarcoFalke)
85ccffa266 test: move releases download incantation to README (Sjors Provoost)
29d6b1da2a test: previous releases: add v0.20.1 (Sjors Provoost)
Pull request description:
Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.
ACKs for top commit:
Sjors:
tACK fa80e10
NelsonGaldeman:
tACK fa80e10d94
Tree-SHA512: 1a1feef823f08c05268759645a8974e1b2d39a024258f5e6acecbe25097aae3fa9302c27262978b40f1aa8e7b525b60c0047199010f2a5d6017dd6434b4066f0
4101ec9d2e doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c1 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075 test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738 net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091e net: distinguish default port per network (Vasil Dimov)
aeac3bce3e net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290c net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e
jonatack:
re-ACK 4101ec9d2e per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce31 net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce31
jonatack:
utACK 2feec3ce31 per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce31, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
b7a8cd9963 [test] submit same txid different wtxid as mempool tx (glozow)
fdb48163bf [validation] distinguish same txid different wtxid in mempool (glozow)
Pull request description:
On master, if you submit a transaction with the same txid but different witness to the mempool, it thinks the transactions are the same. Users submitting through `BroadcastTransaction()` (i.e. `sendrawtransaction` or the wallet) don't get notified that there's a different transaction in the mempool, although it doesn't crash. Users submitting through `testmempoolaccept()` will get a "txn-already-in-mempool" error.
This PR simply distinguishes between `txn-already-in-mempool` and `txn-same-nonwitness-data-in-mempool`, without handling them differently: `sendrawtransaction` still will not throw, but `testmempoolaccept` will give you a different error.
I believe the intention of #19645 is to allow full swaps of transactions that have different witnesses but identical nonwitness data. Returning a different error message + adding a test was suggested: https://github.com/bitcoin/bitcoin/pull/19645#issuecomment-705109193 so this is that PR.
ACKs for top commit:
naumenkogs:
ACK b7a8cd9963
jnewbery:
Code review ACK b7a8cd9963
theStack:
Code-review ACK b7a8cd9963
darosior:
re-utACK b7a8cd9963
Tree-SHA512: 9c6591edaf8727ba5b4675977adb8cbdef7288584003b6cd659828032dc92d2ae915800a8ef8b6fdffe112c1b660df72297a3dcf2e2e3e1f959c6cb3678c63ee
905d672b74 test: use script_util helpers for creating P2W{PKH,SH} scripts (Sebastian Falbesoner)
285a65ccfd test: use script_util helpers for creating P2SH scripts (Sebastian Falbesoner)
b57b633b94 test: use script_util helpers for creating P2PKH scripts (Sebastian Falbesoner)
61b6a017a9 test: wallet util: fix multisig P2SH-P2WSH script creation (Sebastian Falbesoner)
Pull request description:
PR #18788 (commit 08067aebfd) introduced functions to generate output scripts for various types. This PR replaces all manual CScript creations in the P2PKH, P2SH, P2WPKH, P2WSH formats with those helpers in order to increase readability and maintainability over the functional test codebase. The first commit fixes a bug in the wallet_util helper module w.r.t. to P2SH-P2WSH script creation (the result is not used in any test so far, hence it can still be seen as refactoring).
The following table shows a summary of the output script patterns tackled in this PR:
| Type | master branch | PR branch |
| ---------- | ------------- | ------------- |
| P2PKH | `CScript([OP_DUP, OP_HASH160, hash160(key), OP_EQUALVERIFY, OP_CHECKSIG])` | `key_to_p2pkh_script(key)` |
| | `CScript([OP_DUP, OP_HASH160, keyhash, OP_EQUALVERIFY, OP_CHECKSIG])` | `keyhash_to_p2pkh_script(keyhash)` |
| P2SH | `CScript([OP_HASH160, hash160(script), OP_EQUAL])` | `script_to_p2sh_script(script)` |
| P2WPKH | `CScript([OP_0, hash160(key)])` | `key_to_p2wpkh_script(key)` |
| P2WSH | `CScript([OP_0, sha256(script)])` | `script_to_p2wsh_script(script)` |
Note that the `key_to_...` helpers can't be used if an invalid key size (not 33 or 65 bytes) is passed, which is the case in some rare instances where the scripts still have to be created manually.
Possible follow-up ideas:
* further simplify by identifying P2SH-wrapped scripts and using `key_to_p2sh_p2wpkh_script()` and `script_to_p2sh_p2wsh_script()` helpers
* introduce and use `key_to_p2pk_script()` helper for P2PK scripts
ACKs for top commit:
rajarshimaitra:
tACK 905d672b74
LarryRuane:
tACK 905d672b74
0xB10C:
ACK 905d672b74
MarcoFalke:
review ACK 905d672b74 🕹
Tree-SHA512: 7ccfe69699bc81168ac122b03536720013355c1b2fbb088355b616015318644c4d1cd27e20c4f56c89ad083ae609add4bc838cf6316794d0edb0ce9cf7fa0fd8
5b4703c6a7 guix: Test security-check sanity before performing them (Carl Dong)
6cf3345297 scripts: adjust test-symbol-check for guix release environment (fanquake)
1946b5f77c scripts: more robustly test macOS symbol checks (fanquake)
a8127b34bc build: Use and test PE binutils with --reloc-section (Carl Dong)
678348db51 guix: Patch binutils to add security-related disable flags (Carl Dong)
9fdc8afe11 devtools: Improve *-check.py tool detection (Carl Dong)
bda62eab38 ci: skip running the Linux test-security-check target for now (fanquake)
d6ef3543ae lint: Run mypy with --show-error-codes (Carl Dong)
Pull request description:
This is #20980 rebased (to include the Boost Process fix), and with an additional commit (892d6897f1e613084aa0517a660eab2412308e6e) to fix running the `test-security-check` target for the macOS build. It should pass inside Guix, as well as when cross-compiling on Ubuntu, or building natively on macOS.
Note that the `test-security-check` may output some warnings (similar too):
```bash
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 11.4) for platform macOS. Using 11.4.
ld: warning: passed two min versions (10.14, 10.14) for platform macOS. Using 10.14.
```
but those can be ignored, and come about due to us passing `-platform_version` when `-mmacosx-version-min` is already part of `CC`.
Guix builds:
```bash
71ed0c7a13a4726300779ffc87f7d271086a2744c36896fe6dc51fe3dc33df2e guix-build-5b4703c6a70d/output/aarch64-linux-gnu/SHA256SUMS.part
9273980a17052c8ec45b77579781c14ab5d189fa25aa29907d5115513dd302b1 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu-debug.tar.gz
9c042179af43c8896eb95a34294df15d4910308dcdba40b2010cd36e192938b8 guix-build-5b4703c6a70d/output/aarch64-linux-gnu/bitcoin-5b4703c6a70d-aarch64-linux-gnu.tar.gz
1ceddecac113f50a952ba6a201cdcdb722e3dc804e663f219bfac8268ce42bf0 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/SHA256SUMS.part
759597c4e925e75db4a2381c06cda9b9f4e4674c23436148676b31c9be05c7aa guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf-debug.tar.gz
34e3b6beabaf8c95d7c2ca0d2c3ac4411766694ef43e00bd9783badbbaf045a7 guix-build-5b4703c6a70d/output/arm-linux-gnueabihf/bitcoin-5b4703c6a70d-arm-linux-gnueabihf.tar.gz
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 guix-build-5b4703c6a70d/output/dist-archive/SKIPATTEST.TAG
3664f6ceee7898caa374281fd877a7597fe491fa2e9f0c174c28d889d60b559c guix-build-5b4703c6a70d/output/dist-archive/bitcoin-5b4703c6a70d.tar.gz
d6bc35ba0750c1440bb32831b8c12cddee62f6dce10fec2650897444c2bf4748 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/SHA256SUMS.part
a836edf6474ba0c16c19bb217549bac7936c1b44306ed512df58f607ee5568f2 guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu-debug.tar.gz
7cc91c6805d5069ca3bd1771e77d95f83eb184b137198cbf84d1d11d0a5c5afe guix-build-5b4703c6a70d/output/powerpc64-linux-gnu/bitcoin-5b4703c6a70d-powerpc64-linux-gnu.tar.gz
93b4cb7b83c4975120ad5de5a92f050f5760a2a3f2c37c204c647f5a581c924a guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/SHA256SUMS.part
2266e2c5d0dafa28c6c057ccfc1c439baeab1d714d8c3f64a83015d2827116d2 guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu-debug.tar.gz
85f41f42c319b83d049d6fd2e2278c07b40a1e28a2eac596427822c0eef9dc3f guix-build-5b4703c6a70d/output/powerpc64le-linux-gnu/bitcoin-5b4703c6a70d-powerpc64le-linux-gnu.tar.gz
1499ca9119926083d8c3714ca10d8d4c8d864cbeee8848fd8445b7a1d081222d guix-build-5b4703c6a70d/output/riscv64-linux-gnu/SHA256SUMS.part
1995fc1a2e45c49d4b0718aff5dcdac931917e8ae9e762fd23f1126abcecc248 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu-debug.tar.gz
266889eb58429a470f0fd7bb123f2ae09b0aef86c47b0390938b3634a8f748a9 guix-build-5b4703c6a70d/output/riscv64-linux-gnu/bitcoin-5b4703c6a70d-riscv64-linux-gnu.tar.gz
cdc3a0dcf80b110443dac5ddf8bc951001a776a651c898c5ea49bb2d487bfe29 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/SHA256SUMS.part
8538d1eab96c97866b24546c453d95822f24cf9c6638b42ba523eb7aa441cb26 guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.dmg
d1b73133f1da68586b07292a8425f7f851e93f599c016376f23728c041cf39cc guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx-unsigned.tar.gz
5ad94c5f8a5f29405955ff3ab35d137de1acc04398d6c8298fb187b57a6e316a guix-build-5b4703c6a70d/output/x86_64-apple-darwin18/bitcoin-5b4703c6a70d-osx64.tar.gz
8c6d7b3f847faa7b4d16ceecf228f26f146ea982615c1d7a00c57f9230a0c484 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/SHA256SUMS.part
d0a8c99750319ad8046cfa132a54e5c13a08351f94439ae9af0f8e5486c2c2ea guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu-debug.tar.gz
d816bb26dd4b0e309f2f576b1cccc6d78743fb2f357daad2da09bb1177330971 guix-build-5b4703c6a70d/output/x86_64-linux-gnu/bitcoin-5b4703c6a70d-x86_64-linux-gnu.tar.gz
65caaa7f648c7eab1eb82c3331a2ca25b8cd4fe41439de55604501e02571de55 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/SHA256SUMS.part
5bf6f7328cbceb0db22a2d7babb07b60cb6dcc19a6db84a1698589b7f5173a06 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win-unsigned.tar.gz
7aabcb56115decef78d3797840b6e49dbc9b202d56f892490e92616fb06fec9e guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-debug.zip
2f369694648ff9dc5ca1261a1e5874b1c7408ccf2802f9caef56c1334e8a5b7c guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64-setup-unsigned.exe
1c1f92513c4aad38419ff49a7b80bf10e6b1eca01ee8c5e3b2acd1768cf1e3d5 guix-build-5b4703c6a70d/output/x86_64-w64-mingw32/bitcoin-5b4703c6a70d-win64.zip
```
ACKs for top commit:
hebasto:
Approach ACK 5b4703c6a7.
Tree-SHA512: 2cd92a245ea64ef7176cf402a1fa5348a9421c30a4d30d01c950c48f6dcc15cf22ce69ffe1657be97e5fccc14bd933d64683c4439b695528ce3dc34d72dda927
1f449586a9 test: add `bad-txns-prevout-null` test to mempool_accept.py (Sebastian Falbesoner)
aa0a5bb70d test: add `bad-txns-prevout-null` test case to invalid_txs.py (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing tests for the reject reason `bad-txns-prevout-null`, which is thrown in the function `CheckTransaction()`: a62fc35a15/src/consensus/tx_check.cpp (L52-L54)
Basically this condition is met for non-coinbase transactions (the code snippet above only hits if `!tx.IsCoinBase()`) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff.
Can be tested by running the functional tests `feature_block.py`, `p2p_invalid_tx.py` and `mempool_accept.py`. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?).
ACKs for top commit:
rajarshimaitra:
tACK 1f449586a9
brunoerg:
tACK 1f449586a9
kristapsk:
ACK 1f449586a9, code looks correct and all tests pass.
Tree-SHA512: 2d4f940a6ac8e0d80d2670c9e1111cbf43ae6ac62809a2ccf17cffee9a41d387ea4d889ee300eb4a407c055b13bfa5d37102a32ed59964a9b6950bd907ba7204
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
This reject reason is triggered for non-coinbase transactions with
a coinbase-like outpoint, i.e. hash=0, n=0xffffffff.
Note that the invalid tx templates are currently used in the
functional tests feature_block.py and p2p_invalid_tx.py.
The constant `MAX_BLOCK_BASE_SIZE` has been removed from the
core implementation years ago due to being confusing and
superfluous, as it is implied by the block weight limit (see
PRs #10618 and #10608). Since there is also no point in
still keeping it in the functional test framework, we switch
to weight-based accounting on the relevant test code parts
and use `MAX_BLOCK_WEIGHT` instead for the block limit
checks.
c4ddee64c7 test: Add test for replacement relay fee check (Antoine Riard)
Pull request description:
This PR adds rename the `reject_reason` of our implementation of BIP125 rule 4 and adds missing functional test coverage. Note, `insufficient fee` is already the `reject_reason` of few others `PreChecks` replacement checks and as such might be confusing.
> The replacement transaction must also pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting. For example, if the minimum relay fee is 1 satoshi/byte and the replacement transaction is 500 bytes total, then the replacement must pay a fee at least 500 satoshis higher than the sum of the originals.
```
// Finally in addition to paying more fees than the conflicts the
// new transaction must pay for its own bandwidth.
CAmount nDeltaFees = nModifiedFees - nConflictingFees;
if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
hash.ToString(),
FormatMoney(nDeltaFees),
FormatMoney(::incrementalRelayFee.GetFee(nSize))));
}
```
ACKs for top commit:
MarcoFalke:
cr ACK c4ddee64c7
glozow:
ACK c4ddee6, one small suggestion if you retouch.
Tree-SHA512: 7c5d1065db6e6fe57a9f083bf051a7a55eb9892de3a2888679d4a6853491608c93b6e35887ef383a9988d14713fa13a0b1d6134b7354af5fd54765f0d4e98568
3efaf83c75 wallet: deactivate descriptor (S3RK)
6737d9655b test: wallet importdescriptors update existing (S3RK)
586f1d53d6 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db1474 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75
meshcollider:
Code review ACK 3efaf83c75
jonatack:
Light ACK 3efaf83c75 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
e6cf0ed92d wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a8 Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8b wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e543 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b83 descriptors: Cache last hardened xpub (Andrew Chow)
cacc391098 Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75c Refactor Cache merging and writing (Andrew Chow)
976b53b085 Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92d
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92d reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92d
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
fa9ebedec3 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
Pull request description:
It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.
Same for the outpoint index.
Both issues were found by fuzzing:
* The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
* The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
ACKs for top commit:
practicalswift:
cr ACK fa9ebedec3: patch looks correct
jamesob:
crACK fa9ebedec3
theStack:
Code review ACK fa9ebedec3
benthecarman:
crACK fa9ebedec3
Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
754f134a50 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d1072 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34 Add OutputType::BECH32M (Andrew Chow)
177c15d2f7 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50
jonatack:
ACK 754f134a50
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
fadddd13ee test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e test: Misc cleanup (MarcoFalke)
fa1668bf50 test: Run pep-8 (MarcoFalke)
facd97ae0f scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK fadddd13ee on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
bdb8b9a347 test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
1914054208 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
6168eb06b2 [test] Prevent intermittent issue (Amiti Uttarwar)
1d8193e2a2 [test] Remove GetAddrStore class (Amiti Uttarwar)
ef2f149bf2 [test] Update GetAddrStore callers to use AddrReceiver (Amiti Uttarwar)
e8c67ea19a [test] Add functionality to AddrReceiver (Amiti Uttarwar)
09dc073cff [test] Allow AddrReceiver to be used more generally (Amiti Uttarwar)
Pull request description:
A test refactor broken out from #21528 & a fix to #22243.
This PR:
1. consolidates the two helper classes into one, with the intent of making the test logic more clear & usable as we add more subtests to the file
2. hopefully fixes the test flakiness by bumping up the mocktime interval to ensure `m_next_addr_send` timer triggers
ACKs for top commit:
mzumsande:
Code-Review ACK 6168eb06b2
lsilva01:
Tested ACK 6168eb06b2 on Ubuntu 20.04
brunoerg:
tACK 6168eb06b2
Tree-SHA512: 248324f9d37e0e5ffe4acc437cd72ad9a2960abc868a97c6040a36e6ea8b59029127ac4f63fcf67d981a5bb4dbf2334bb2c23c541fae8e910d5523884bcedcba
d637a9b397 Taproot descriptor inference (Pieter Wuille)
c7388e5ada Report address as solvable based on inferred descriptor (Pieter Wuille)
29e5dd1a5b consensus refactor: extract ComputeTapleafHash, ComputeTaprootMerkleRoot (Pieter Wuille)
Pull request description:
Includes:
* First commit from #21365, adding TaprootSpendData in SigningProvider
* A refactor to expose ComputeTapleafHash and ComputeTaprootMerkleRoot from script/interpreter
* A tiny change to make `getaddressinfo` report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
* Logic to infer the script tree back from TaprootSpendData, and then use that to infer descriptors.
ACKs for top commit:
achow101:
re-ACK d637a9b397
Sjors:
re-utACK d637a9b
meshcollider:
Code review ACK d637a9b397
Tree-SHA512: 5ab9b95da662382d8549004be4a1297a577d7caca6b068f875c7c9343723931d03fa9cbf133de11f83b74e4851490ce820fb80413c77b9e8495a5f812e505d86
bb719a08db style: remove () from assert in rpc_setban.py (Vasil Dimov)
24b10ebda3 doc: fix grammar in doc/files.md (Vasil Dimov)
dd4e957dcd test: ensure banlist can be read from disk after restart (Vasil Dimov)
d197977ae2 banman: save the banlist in a JSON format on disk (Vasil Dimov)
Pull request description:
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
ACKs for top commit:
jonatack:
Code review re-ACK bb719a08db per `git range-diff 6a67366 4b52c72 bb719a0`
achow101:
Code Review ACK bb719a08db
Tree-SHA512: fc135c3a1fe20bcf5d008ce6bea251b4135e56c78bf8f750b4bd8144c095b81ffe165133cdc7e4715875eec7e7c4e13ad9f5d2450b21102af063d7c8abf716b6
fafd9165e9 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)
Pull request description:
Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)
ACKs for top commit:
jamesob:
crACK fafd9165e9
Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
The `on_addr` functionality of `AddrReceiver` tests logic specific to how the
addr messages are set up in the test bodies. To allow other callers to also use
`AddrReceiver`, only apply the assertion logic if the caller indicates
desirability by setting `test_addr_contents` to true when initializing the
class.
testshell in jupyter was failing due to an extra arg.
this adds a dummy -f param, which allows TestShell to
be used in a command line or jupyter environment
d6d2ab9845 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0e test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab9845
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
b9e76f1bf0 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7d rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef6 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59 rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf0
promag:
ACK b9e76f1bf0.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.
This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
fa7d71f270 test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649 test: Remove unused generate() from test (MarcoFalke)
faff3f35b7 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups 🎨
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270 thanks for the explanations, nicely done
theStack:
ACK fa7d71f270🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
55311197c4 Added new test for future blocks reacceptance (sanket1729)
511a5af462 Fixed inconsistencies between code and comments (sanket1729)
Pull request description:
This Commit does 3 things:
1) Adds a test case for checking reacceptance a previously rejected block which
was too far in the future.
~~2) clean up uses of rehash or calc_sha256 where it was not needed~~
3) While constructing block 44, this commit makes the code consistent with the expected figure in
the comment just above it by adding a transaction to the block.
4) Fix comment describing `sign_tx()` function
ACKs for top commit:
duncandean:
reACK 5531119
brunoerg:
reACK 55311197c4
Tree-SHA512: d40c72fcdbb0b2a0715adc58441eeea08147ee2ec5e371a4ccc824ebfdc6450698bd40aaeecb7ea7bfdb3cd1b264dd821b890276fff8b8d89b7225cdd9d6b546
451b96f7d2 test: kill process group to avoid dangling processes (S3RK)
Pull request description:
This is an alternative to #19281
This PR fixes a problem when after test failure with `--failfast` option there could be dangling nodes. The nodes will continue to occupy rpc/p2p ports on the machine and will cause further test failures.
If there are any dangling nodes left at the end of the test run we kill the whole process group.
Pros: the operations is immediate and won't lead to CI timeout
Cons: the test_runner process is also killed and exit code is 137
Example output:
```
...
Early exiting after test failure
TEST | STATUS | DURATION
rpc_decodescript.py | ✓ Passed | 2 s
rpc_deprecated.py | ✓ Passed | 2 s
rpc_deriveaddresses.py | ✓ Passed | 2 s
rpc_dumptxoutset.py | ✖ Failed | 2 s
ALL | ✖ Failed | 8 s (accumulated)
Runtime: 4 s
Killed: 9
> echo $?
137
```
ACKs for top commit:
MarcoFalke:
review ACK 451b96f7d2
aitorjs:
ACK 451b96f7d2. Manual testing with and without **--failfast**.
Tree-SHA512: 87e510a1411b9e7571e63cf7ffc8b9a8935daf9112ffc0f069d6c406ba87743ec439808181f7e13cb97bb200fad528589786c47f0b43cf3a2ef0d06a23cb86dd
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
754e802274 test: check rejected future block later accepted (Luke Dashjr)
Pull request description:
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
This PR re-opens https://github.com/bitcoin/bitcoin/pull/17872 which went stale and addresses the nits raised by reviewers there.
ACKs for top commit:
MarcoFalke:
review ACK 754e802274
Tree-SHA512: a2bbc8fffb523cf2831e1ecb05f20868e30106a38cc2e369e4973fa549cca06675a668df16f76c49cc4ce3a22925404255e5c53c4232d63ba1b9fca878509aa0
1) Makes the code for block 44 consistent with the expected figure in
the comment above it by adding a transaction to the block
2) Fixed comment describing sign_tx() function
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
Co-authored-by: Will Clark <will8clark@gmail.com>
01eedf3821 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner)
6e63e366d6 test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner)
Pull request description:
Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:
* pass a list of parent_txids/vouts instead of single values
* always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one)
This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.
ACKs for top commit:
mjdietzx:
reACK 01eedf3821
klementtan:
Code review ACK 01eedf3821
MarcoFalke:
review ACK 01eedf3821🙅
Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0
fbf485c9b2 Allow tr() import only when Taproot is active (Andrew Chow)
Pull request description:
To avoid issues around fund loss, only allow descriptor wallets to import `tr()` descriptors after taproot has activated.
ACKs for top commit:
sipa:
utACK fbf485c9b2
fjahr:
Code review ACK fbf485c9b2
laanwj:
Code review ACK fbf485c9b2
prayank23:
utACK fbf485c9b2
Tree-SHA512: 83c43376515eea523dbc89bc5a0fde53e54aec492e49a40c2a33d80fc94aac459e232ae07b024b4bd75b58078c8d090bc7a2d69541c5d3d4834d2f4cfc9c8208
In order to enable exact fee calculation for transactions that spend
P2PK outputs in the MiniWallet, we enforce the created signatures to
have a fixed length (>49.89% probability) by default. With that it is
easier to check the created transactions vsize and avoid flaky tests
that would appear whenever the signatures R- or S-values are smaller
(due to leading zero bytes).
Note that to get the total scriptSig size one has to add another
2 bytes, as there is also the OP_PUSHx instruction on the front and
the sighash type byte on the back, leading to a final scriptSig size
of 73 bytes.
2eb0eeda39 validation: document lack of inherited signaling in RBF policy (Antoine Riard)
906b6d9da6 test: Extend feature_rbf.py with no inherited signaling (Antoine Riard)
Pull request description:
Contrary to BIP125 or other full-node implementation (e.g btcd), Bitcoin Core's mempool policy doesn't implement inherited signaling.
This PR documents our mempool behavior on this and add a test demonstrating the case.
ACKs for top commit:
jonatack:
ACK 2eb0eeda39
benthecarman:
ACK 2eb0eeda39
Tree-SHA512: d41453d3b49bae3c1eb532a968f43bc047084913bd285929d4d9cba142777ff2be38163d912e28dfc635f4ecf446de68effad799c6e71be52f81e83410c712fb
Add an explicit sync_blocks call in wallet_orphanedreward.py, which was
missing and could lead to intermittent failures of the test due to
race conditions.
This will presumably fix https://github.com/bitcoin/bitcoin/issues/22181.
faa94961d6 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
68ace23fa3 test: convert docs into type annotations in test_framework/test_node.py (fanquake)
8bfcba36db test: convert docs into type annotations in test_framework/wallet.py (fanquake)
b043ca8e8b test: convert docs into type annotations in test_framework/util.py (fanquake)
Pull request description:
Rather than having function types exist as documentation, make them type annotations, which enables more `mypy` checking.
ACKs for top commit:
instagibbs:
utACK 68ace23fa3
Tree-SHA512: b705f26b48baabde07b9b2c0a8d547b4dcca291b16eaf5ac70462bb3a1f9e9c2783d93a9d4290889d0cbb3f7db3671446754411a1f498b265d336f6ff33478df
62cb8d98d2 qt: Drop BitcoinGUI* WalletFrame data member (Hennadii Stepanov)
f73e5c972a qt: Move CreateWalletActivity connection from WalletFrame to BitcoinGUI (Hennadii Stepanov)
20e2e24e90 qt: Move WalletView connections from WalletFrame to BitcoinGUI (Hennadii Stepanov)
Pull request description:
This PR:
- implements an idea from https://github.com/bitcoin/bitcoin/pull/17937#issuecomment-575991765
- simplifies `WalletFrame` class interface
- as a side effect, removes `bitcoingui` -> `walletframe` -> `bitcoingui` circular dependency
- is an alternative to https://github.com/bitcoin/bitcoin/pull/17500
ACKs for top commit:
promag:
Tested ACK 62cb8d98d2 on macos 11.2.3 with depends build.
jarolrod:
ACK 62cb8d98d2
Tree-SHA512: 633b526a8499ba9ab4b16928daf4de4f6d610284bb9fa51891cad35300a03bde740df3466a71b46e87a62121330fcc9e606eac7666ea5e45fa6d5785b60dcbbd
fa2b6c62cd test: Remove unused node from feature_nulldummy (MarcoFalke)
Pull request description:
This is confusing and might even slow down the test.
This reverts a change that was added a year ago in d438d60 and then the need for it was removed by 95d5d5e six months ago.
Top commit has no ACKs.
Tree-SHA512: 9a86792e9a634cf7bbd4e7a21b1acdfc3baba1b1962fe2b9b73848436d10351d2326dca01313c097ba2342dde7207add73e731d053c0bfa888a5d8f2b233a7cf
e4356f6a6c Testcase for wallet issue with orphaned rewards. (Daniel Kraft)
Pull request description:
This adds a new test case demonstrating the wallet issue when block rewards are orphaned (#14148).
ACKs for top commit:
LarryRuane:
ACK e4356f6a6c
leonardojobim:
reACK e4356f6a6c .
Tree-SHA512: e9a2310ee1b3d52cfa302f431ed3d272bbc1b9195439ff318d9eb1006c0b28968dbe840e1600b6ff185e5d7ea57e4dcc837cef16051b5537445e10bc363b8c22
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
5d82a57db4 contrib: remove torv2 seed nodes (Jon Atack)
5f7e086dac contrib: update generate-seeds.py to ignore torv2 addresses (Jon Atack)
8be56f0f8e p2p, refactor: extract OnionToString() from CNetAddr::ToStringIp() (Jon Atack)
5f9d3c09b4 p2p: remove torv2 from CNetAddr::ToStringIP() (Jon Atack)
3d39042144 p2p: remove torv2 in SetIP() and ADDR_TORV2_SIZE constant (Jon Atack)
cff5ec477a p2p: remove pre-addrv2 onions from SerializeV1Array() (Jon Atack)
4192a74413 p2p: ignore torv2-in-ipv6 addresses in SetLegacyIPv6() (Jon Atack)
1d631e956f p2p: remove BIP155Network::TORV2 from GetBIP155Network() (Jon Atack)
7d1769bc45 p2p: remove torv2 from SetNetFromBIP155Network() (Jon Atack)
eba9a94b9f fuzz: rename CNetAddr/CService deserialize targets (Jon Atack)
c56a1c9b18 p2p: drop onions from IsAddrV1Compatible(), no longer relay torv2 (Jon Atack)
f8e94002fc p2p: remove torv2/ADDR_TORV2_SIZE from SetTor() (Jon Atack)
0f1c58ae87 test: update feature_proxy to torv3 (Jon Atack)
Pull request description:
This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.
- no longer serialize/deserialize and relay Tor v2 addresses
- ignore incoming Tor v2 addresses
- remove Tor v2 addresses from the addrman and peers.dat on node launch
- update generate-seeds.py to ignore Tor v2 addresses
- remove Tor v2 hard-coded seeds
Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in https://github.com/bitcoin/bitcoin/issues/21351
```
<bitcoind debug log>
Socks5() connect to […].onion:8333 failed: general failure
<tor log>
Invalid hostname [scrubbed]; rejecting
```
and the addrman no longer has Tor v2 addresses on launching bitcoind.
```rake
$ ./src/bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 44483,
"ipv6": 8467,
"torv2": 0,
"torv3": 2296,
"i2p": 6,
"total": 55252
}
}
```
After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.
Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.
Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers: `rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion`
Tested using `addnode`, `getaddednodeinfo`,`addpeeraddress`, `disconnectnode` and `-addrinfo` that a currently valid, connectable Tor v2 peer can no longer be added:
Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.
ACKs for top commit:
laanwj:
Code review ACK 5d82a57db4
Tree-SHA512: 590ff3d2f6ef682608596facb4b01f44fef69716d2ab3552ae1655aa225f4bf104f9ee08d6769abb9982a8031de93340df553279ce1f5023771f9f2b651178bb
44d05d0a69 test: remove sanitizer suppression for nanobench (Martin Ankerl)
e3c866e3ca test: update nanobench from release 4.0.0 to 4.3.4 (Martin Ankerl)
Pull request description:
This updates the third-party library nanobench with the latest release. It contains mostly minor bugfixes, a new pyperf output format, ability to suppress warnings with environment variable `NANOBENCH_SUPPRESS_WARNINGS`. Full changelog:
v4.0.2
* Changed `doNotOptimizeAway` to what google benchmark is doing. The old code did not work on some machines.
* fix: display correct "total" value
* minor Documentation updates
v4.1.0
* Updated link to new pyperf home
* Adds ability to configure console output time unit
* Add support for environment variable `NANOBENCH_SUPPRESS_WARNINGS`
* Nanobench is now usable with CMake's FetchContent (see documentation: https://nanobench.ankerl.com/tutorial.html#cmake-integration)
v4.2.0
* Ability to store and later compare results added, through `pyperf`.
* See https://nanobench.ankerl.com/tutorial.html#pyperf-python-pyperf-module-output
* Added lots of build targets to travis, similar to bitcoin's build.
* Some minor API & documentation improvements
v4.3.0
* `ankerl::nanobench::Rng` can now return the state with `std::vector<uint64_t> Rng::state()`, and this can also be used to initialize the Rng.
v4.3.1
* Minor cmake improvements when integrationg as a third-party library: add alias `nanobench::nanobench`, default to C++17
v4.3.2
* Fixed a MSVC 2015 build problem
* updates license to 2021.
* build should now work with very old linux headers
* Also disable UBSAN (bitcoin needed to add a suppression)
v4.3.3
* Do not use locale-dependent `std::to_string`
v4.3.4
* Add missing sanitizer suppression to `rotl`
ACKs for top commit:
MarcoFalke:
review ACK 44d05d0a69
Tree-SHA512: 3291c85057720cfc84a44bfaa305a7d0df4dc35779169d20de73d32e40d4cdbf3f005bf343f79710eca517441de2459e8118c195c5f5136f99d1f50ebd5dfd08
a3f0cbf82d test: run mempool_reorg.py even with wallet disabled (Darius Parvin)
Pull request description:
Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.
As part of this PR I created a new method in `MiniWallet`, `create_self_transfer`, to return a raw tx (without broadcasting it) and its associated utxo.
ACKs for top commit:
MarcoFalke:
cr ACK a3f0cbf82d
Tree-SHA512: 316a38faffadcb87499c1d6eca21e9696cef65362bbffcf621788a9b771bb1fa2971b1c7835cbd34b952d7612ad83afbca824cd8be39ecd6b994e8963027f991
In #21738 an ASAN suppression for nanobench was added. This is not needed any more,as nanobench.h already includes the necessary suppressions for the relevant code.
The functions cltv_modify_tx(), cltv_invalidate() and cltv_validate()
all modify the passed transaction in-place, i.e. there is no need
to return a copy.
- run mempool_reorg.py even when the wallet is not compiled
- add `locktime` argument to `create_self_transfer` and `send_self_transfer`
- use more logs instead of comments
bfa9309ad6 Use COINBASE_MATURITY constant in functional tests. (Kiminuo)
525448df9d Move COINBASE_MATURITY from `feature_nulldummy` test to `blocktools`. (Kiminuo)
Pull request description:
`COINBASE_MATURITY` constant was added to `feature_nulldummy` test in #21373. This PR moves the constant to `blocktools.py` file and uses the constant in more tests as suggested [here](https://github.com/bitcoin/bitcoin/pull/21373#discussion_r605418462).
Edit: Goal of this PR is to replace integer constants with `COINBASE_MATURITY` but not necessarily in *all* cases because that would mean to read and fully understand all tests. That's out of my time constraints. Any reports where `COINBASE_MATURITY` should be used are welcome though!
ACKs for top commit:
theStack:
ACK bfa9309ad6🌇
Tree-SHA512: 01f04645f05a39028681f355cf3d42dd63ea3303f76d93c430e0fdce441934358a2d847a54e6068d61932f1b75e1d406f51859b057b3e4b569f7083915cb317f
2be3572506 test: Fix IPv6 check on BSD systems (nthumann)
Pull request description:
I noticed that `test_ipv6_local()` always returns `False` on macOS or FreeBSD, even though IPv6 is working perfectly fine. This causes `test/functional/rpc_bind.py --ipv6` and `test/functional/feature_proxy.py` to skip their run.
Apparently, there's a check if the port number is `0` (see [here](64881da478/sys/netinet6/udp6_usrreq.c (L248)) or [here](8f02f2a044/bsd/netinet6/udp6_usrreq.c (L282))), while Linux has no problem with this.
This is fixed by specifying any other port number than `0`, e.g. `1`. Still, because of `SOCK_DGRAM`, no actual connection is made.
ACKs for top commit:
fanquake:
ACK 2be3572506 - nice improvement. I checked that with this change ipv6 related tests in `feature_proxy.py` are being run.
theStack:
ACK 2be3572506
Tree-SHA512: 8417c2d3cf71050529f3fa409a03872040fe5d249eae4172f276e62156e505a20d375b963712a186c9ad7967d8a497b5900d327c74a9693f68c33063871d4691
c7bd5842e4 MOVEONLY: CWallet transaction code out of wallet.cpp/.h (Russell Yanofsky)
Pull request description:
This commit just moves function without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra`
Motivation for this change is to make `wallet.cpp/h` less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems.
This moves wallet classes and methods that deal with transactions out of `wallet.cpp/.h` into better organized files:
- `transaction.cpp/.h` - CWalletTx and CMerkleTx class definitions
- `receive.cpp/.h` - functions checking received transactions and computing balances
- `spend.cpp/.h` - functions creating transactions and finding spendable coins
After #20773, when loading is separated from syncing it will also be possible to move more `wallet.cpp/.h` functions to:
- `sync.cpp/.h` - functions handling chain notifications and rescanning
This commit arranges `receive.cpp` and `spend.cpp` functions in dependency order so it's possible to skim `receive.cpp` and get an idea of how computing balances works, and skim `spend.cpp` and get an idea of how transactions are created, without having to jump all over `wallet.cpp` where functions are not in order and there is a lot of unrelated code.
Followup commit "refactor: Detach wallet transaction methods" in https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
ACKs for top commit:
Sjors:
re-utACK c7bd5842e4
fjahr:
utACK c7bd5842e4
promag:
Code review ACK c7bd5842e4, verified move only claim.
meshcollider:
Dimmed-zebra-check and functional test run ACK c7bd5842e4
Tree-SHA512: 4981de6911cb1196774db375494355cc9af59b52456129c002d264a77cd9ed6175f8ecbb6b2f492a59a4d5a0def21a39d96fa79c9f4d99be0992985f553be32f
13650fe2e5 [policy] detect unsorted packages (glozow)
9ef643e21b [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7e [test] functional test for packages in RPCs (glozow)
9ede34a6f2 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709 [policy] limit package sizes (glozow)
c9e1a26d1f [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916c [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941d [validation] package validation for test accepts (glozow)
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5 [policy] Define packages (glozow)
249f43f3cc [refactor] add option to disable RBF (glozow)
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df [validation] make CheckSequenceLocks context-free (glozow)
Pull request description:
This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.
**Motivation:**
- This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes#18480.
- It's also a first step towards package validation in a minimally invasive way.
- The RPC commit happens to close#21074 by clarifying the "allowed" key.
There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
- No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
- The package cannot conflict with the mempool, i.e. RBF is disabled.
- The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)
ACKs for top commit:
laanwj:
Code review re-ACK 13650fe2e5
jnewbery:
Code review ACK 13650fe2e5
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
This commit just moves functions without making any changes. It can be
reviewed with `git log -p -n1 --color-moved=dimmed_zebra`
Motivation for this change is to make wallet.cpp/h less monolithic and
start to make wallet transaction state tracking comprehensible so bugs
in
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
can be fixed safely without introducing new problems.
This commit moves wallet classes and methods that deal with transactions
out of wallet.cpp/.h into better organized files:
- transaction.cpp/.h - CWalletTx and CMerkleTx class definitions
- receive.cpp/.h - functions checking received transactions and computing balances
- spend.cpp/.h - functions creating transactions and finding spendable coins
After #20773, when loading is separated from syncing it will also be
possible to move more wallet.cpp/.h functions to:
- sync.cpp/.h - functions handling chain notifications and rescanning
This commit arranges receive.cpp and spend.cpp functions in dependency
order so it's possible to skim receive.cpp and get an idea of how
computing balances works, and skim spend.cpp and get an idea of how
transactions are created, without having to jump all over wallet.cpp
where functions are not in order and there is a lot of unrelated code.
Followup commit "refactor: Detach wallet transaction methods" in
https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and
tweaks function names and arguments to reflect new locations. The two
commits are split into separate PRs because this commit is more work to
maintain and less work to review, while the other commit is less work to
maintain and more work to review, so hopefully this commit can be merged
earlier.
e6fe1c37d0 rpc: Improve avoidpartialspends and avoid_reuse documentation (Fabian Jahr)
8f073076b1 wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 (Fabian Jahr)
Pull request description:
Follow-up to #17824.
This increases OUTPUT_GROUP_MAX_ENTRIES to 100 which means that OutputGroups will now be up to 100 outputs large, up from previously 10. The main motivation for this change is that during the PR review club on #17824 [several participants signaled](https://bitcoincore.reviews/17824.html#l-339) that 100 might be a better value here.
I think fees should be manageable for users but more importantly, users should know what they can expect when using the wallet with this configuration, so I also tried to clarify the documentation on `-avoidpartialspends` and `avoid_reuse` a bit. If there are other additional ways how or docs where users can be made aware of the potential consequences of using these parameters, please let me know. Another small upside is that [there seem to be a high number of batching transactions with 100 and 200 inputs](https://miro.medium.com/max/3628/1*sZ5eaBSbsJsHx-J9iztq2g.png)([source](https://medium.com/@hasufly/an-analysis-of-batching-in-bitcoin-9bdf81a394e0)) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.
ACKs for top commit:
jnewbery:
ACK e6fe1c37d0
Xekyo:
retACK e6fe1c37d0
rajarshimaitra:
tACK `e6fe1c3`
achow101:
ACK e6fe1c37d0
glozow:
code review ACK e6fe1c37d0
Tree-SHA512: 79685c58bafa64ed8303b0ecd616fce50fc9a2b758aa79833e4ad9f15760e09ab60c007bc16ab4cbc4222e644cfd154f1fa494b0f3a5d86faede7af33a6f2826
Only allow "packages" with no conflicts, sorted in order of dependency,
and no more than 25 for now. Note that these groups of transactions
don't necessarily need to adhere to some strict definition of a package
or have any dependency relationships. Clients are free to pass in a
batch of 25 unrelated transactions if they want to.
For the MiniWallet constructor, the two boolean parameters
"raw_script" and "use_p2pk" are replaced by a single parameter of the
newly introduced type MiniWalletMode (derived by enum.Enum), which can
hold the following values:
- ADDRESS_OP_TRUE
- RAW_OP_TRUE
- RAW_P2PK
4bea301692 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea301692
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
Using the MiniWallet in P2PK mode, all transactions submitted to the
mempool are following the standard policy now, i.e. the node command
line parameter '-acceptnonstdtxn=1' is not needed anymore.
9c891b64ff net: initialize nMessageSize to max uint32_t instead of -1 (eugene)
Pull request description:
nMessageSize is uint32_t and is set to -1. This will warn with `-fsanitize=implicit-integer-sign-change` when V1TransportDeserializer calls into the ctor. This pull initializes nMessageSize to `numeric_limits<uint32_t>::max()` instead and removes the ubsan suppression.
ACKs for top commit:
laanwj:
Code review ACK 9c891b64ff
promag:
Code review ACK 9c891b64ff.
Tree-SHA512: f05173d9553a01d207a5a7f8ff113d9e11354c50b494a67d44d3931c151581599a9da4e28f40edd113f4698ea9115e6092b2a5b7329c841426726772076c1493
bd7f27d16d refactor: feature_csv_activation.py: move tx helper functions to methods (Sebastian Falbesoner)
2eca46b0aa test: use MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_csv_activation.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Short reviewers guideline:
- Since we exclusively work with anyone-can-spend outputs here (raw scriptPubKey = OP_TRUE), signing is not needed anymore. The function `sign_transaction` and its calls are removed, after changing a tx (e.g. its scriptSig or nVersion) a simple `.rehash()` call is sufficient. Also, generating an address `self.nodeaddress` (and with that, passing it to the the various test tx creation/sending helper methods) is not needed anymore and removed.
- The test repeatedly uses the same input for creating different txs (e.g. with different txversions 1 and 2). To let `MiniWallet` create a tx with a specific input, we have to call `.get_utxo()` before which also marks the UTXO as spent. The method is changed to also support keeping the UTXO in its internal list (`mark_as_spent=False`). With the behaviour on master, the second call to `.get_utxo()` with the same input would fail.
- To keep the diff in the first commit short, the `miniwallet` is set as a global variable, to avoid passing it on every tx creation/spending helper. The global is eliminated in the second (refactoring) commit, where all the helpers are moved to the test class as methods. By that, we can use `self.nodes[0]` directly in the helpers and don't have to pass it again and again. I think there could still be a lot of improvements/refactoring done in the test, but that should hopefully serve as a good basis.
ACKs for top commit:
laanwj:
Code review ACK bd7f27d16d
MarcoFalke:
review ACK bd7f27d16d🐕
Tree-SHA512: 24fb6a0f7702bae40d5271d197119827067d4b597e954d182e4c1aa5d0fa870368eb3ffed469b26713fa8ff8eb3ecc06abc80b2449cd68156d5559e7ae8a2b11
11d6459b6e rpc: include_unsafe option for fundrawtransaction (t-bast)
Pull request description:
Allow RPC users to opt-in to unsafe inputs when funding a raw transaction.
Applications that need to manage a complex RBF flow (such as lightning nodes using anchor outputs) are very limited if they can only use safe inputs.
I also added this option to `send` and `walletcreatefundedpsbt` who internally delegate to `fundrawtransaction`.
Fixes#21299
ACKs for top commit:
laanwj:
Code review ACK 11d6459b6e
Tree-SHA512: 5e542a4febcfd6f41cf784678ff02ec9282eae2082c274983f72c5ea87b7ebbe1bd5fdc6a020d7a9d5996157754eb4966b8aeb6c1ceebf0b1519f735579b8bac
847288df07 test: fee rate values that cannot be represented as sat/vB (Jon Atack)
06a90fa038 rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 (Jon Atack)
0742c7840f rpc: enable passing decimals to AmountFromValue, add doxygen (Jon Atack)
8ce3ef57a3 test: ParseFixedPoint with 3 decimals for sat/vB fee rates (Jon Atack)
b503327597 test: type error and out of range fee rates where missing (Jon Atack)
c5fd4344f7 test: explicit fee rates with invalid amounts (Jon Atack)
ea6f76b66e test: improve zero-value explicit fee rate coverage (Jon Atack)
Pull request description:
- Improve/close gaps in existing test coverage before making the change
- Enable passing `decimals` to `ParseFixedPoint()` when calling `AmountFromValue()`
- Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise
- Add regression test coverage
Closes#20534.
ACKs for top commit:
MarcoFalke:
review ACK 847288df07🔷
Tree-SHA512: c539d07ae9b21c0d6c8ea460beb9c8dad5559445518aace560abc3c05c588907bae189b6fd7602b3b397de4a42356136c3ec6f960d3dcf2d5d16377aef4ab5a2
This allows to get rid of the global miniwallet variable and to specify
the used node self.nodes[0] at only one place, instead of passing it to
every tx creation/send method again and again.
Can be reviewed with --ignore-all-space --color-moved=dimmed-zebra
09205b33aa net: Clarify message header validation errors (W. J. van der Laan)
955eee7680 net: Sanitize message type for logging (W. J. van der Laan)
Pull request description:
- Use `SanitizeString` when logging message errors to make sure that the message type is sanitized. I have checked all logging in `net.cpp`.
- For the `MESSAGESTART` error don't inspect and log header details at all: receiving invalid start bytes makes it likely that the packet isn't even formatted as valid P2P message. Logging the four unexpected start bytes (as hex) should be enough.
- Update `p2p_invalid_messages.py` test to check this.
- Improve error messages in a second commit.
Issue reported by gmaxwell.
ACKs for top commit:
MarcoFalke:
re-ACK 09205b33aa only change is log message fixup 🔂
practicalswift:
re-ACK 09205b33aa
Tree-SHA512: 8fe5326af135cfcf39ea953d9074a8c966b9b85a810b06a2c45b8a745cf115de4f321e72fc769709d6bbecfc5953aab83176db6735b04c0bc6796f59272cadce
2227fc4e62 test: minor fixes & improvements for files linter test (windsok)
Pull request description:
Couple of minor fixes & improvements for files linter test added in #21740
- Use a context manager when opening files, so that files are closed are we are done with them
- Use the `-z` flag when shelling out to `git ls-files` so that we can catch newlines and other weird control characters in filenames.
From the `git ls-files` manpage:
```
-z \0 line termination on output and do not quote filenames. See OUTPUT below for more information.
Without the -z option, pathnames with "unusual" characters are quoted as explained for the configuration variable
core.quotePath (see git-config(1)). Using -z the filename is output verbatim and the line is terminated by a NUL byte.
```
ACKs for top commit:
MarcoFalke:
cr ACK 2227fc4e62
practicalswift:
cr ACK 2227fc4e62: patch looks correct
Tree-SHA512: af059a805f4a7614162de85dea856052a45ab531895cb0431087e7fc9e037513fa7501bb5eb2fe43238adf5f09e77712ebdbb15b1486983359ad3661a3da0c60
Updates the lint-files.py lint test:
* Use a context manager when opening files, so that files are closed.
* Use the -z flag when shelling out to git ls-files so that we can catch newlines
and other weird control characters in filenames
- Use `SanitizeString` when logging message errors to make sure that the
message type is sanitized.
- For the `MESSAGESTART` error don't inspect and log header details at
all: receiving invalid start bytes makes it likely that the packet isn't
even formatted as valid P2P message. Logging the four unexpected start
bytes should be enough.
- Update `p2p_invalid_messages.py` test to check this.
Issue reported by gmaxwell.
9f767e8438 test: use MiniWallet for p2p_blocksonly.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (p2p_blocksonly.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead, as proposed in #20078.
Note that MiniWallet creates segwit transactions by default, i.e. txid and wtxid are not identical and we have to return both from `check_p2p_tx_violation(...)`: wtxid is needed to match an expected `"received getdata for: wtx ..."` debug output, whereas the txid is needed to wait for a certain tx via `wait_for_tx(...)`.
ACKs for top commit:
jonatack:
ACK 9f767e8438 tested with `--disable-wallet`
Tree-SHA512: f08001f02c3c310ccdf713af0ba17304368a36414f412749908bbe8c03ad1e902190b8768b79f3b4909855762f285e7ab1b627cc4f45c90b42bb097a43cb4318
fa03d0acd6 fuzz: Create a block template in tx_pool targets (MarcoFalke)
fa61ce5cf5 fuzz: Limit mocktime to MTP in tx_pool targets (MarcoFalke)
fab646b8ea fuzz: Use correct variant of ConsumeRandomLengthString instead of hardcoding a maximum size (MarcoFalke)
fae2c8bc54 fuzz: Allow to pass min/max to ConsumeTime (MarcoFalke)
Pull request description:
Relatively simple check to ensure a block can always be created from the mempool
ACKs for top commit:
practicalswift:
Tested ACK fa03d0acd6
Tree-SHA512: e613376ccc88591cbe594db14ea21ebc9b2b191f6325b3aa4ee0cd379695352ad3b480e286134ef6ee30f043d486cf9792a1bc7e44445c41045ac8c3b931c7ff
91d93aac4e validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)
Pull request description:
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410
ACKs for top commit:
Sjors:
utACK 91d93aac4e
ryanofsky:
Code review ACK 91d93aac4e. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
46b025e00d test: add new python linter to check file names and permissions (windsok)
6f6bb3ebc7 test: fix file permissions on various scripts (windsok)
Pull request description:
Adds a new python linter test which tests for correct filenames and file permissions in the repository.
Replaces the existing tests in the `test/lint/lint-filenames.sh` and `test/lint/lint-shebang.sh` linter tests, as well as adding some new and increased testing. This increased coverage is intended to catch issues such as in #21728 and https://github.com/bitcoin/bitcoin/pull/16807/files#r345547050
Summary of tests:
* Checks every file in the repository against an allowed regexp to make sure only lowercase or uppercase alphanumerics (a-zA-Z0-9), underscores (_), hyphens (-), at (@) and dots (.) are used in repository filenames.
* Checks only source files (*.cpp, *.h, *.py, *.sh) against a stricter allowed regexp to make sure only lowercase alphanumerics (a-z0-9), underscores (_), hyphens (-) and dots (.) are used in source code filenames. Additionally there is an exception regexp for directories or files which are excepted from matching this regexp (This should replicate the existing `test/lint/lint-filenames.sh` test)
* Checks all files in the repository match an allowed executable or non-executable file permission octal. Additionally checks that for executable files, the file contains a shebang line.
* Checks that for executable `.py` and `.sh` files, the shebang line used matches an allowable list of shebangs (This should replicate the existing `test/lint/lint-shebang.sh` test)
* Checks every file that contains a shebang line to ensure it has an executable permission
Additionally updates the permissions on various files to comply with the new tests.
Fixes#21729
ACKs for top commit:
practicalswift:
cr re-ACK 46b025e00d: patch still looks correct
kiminuo:
code review ACK 46b025e00d if `contrib/gitian-descriptors/assign_DISTNAME` permission change is deemed OK.
laanwj:
Code review ACK 46b025e00d
Tree-SHA512: 1c8201a2cee0d9cbce15652b68cec9a6458a8b493fcd5392f98560aca0b1a12e668baab65a47100f116f626dadc3f591deb47f7368468c6a46c6c712c2533455
f09e6b2585 test: fix off-by-ones in rpc_fundrawtransaction (Jon Atack)
Pull request description:
The variables in these assertions should be the same within each line.
ACKs for top commit:
laanwj:
ACK f09e6b2585
theStack:
ACK f09e6b2585
Tree-SHA512: 7ac754eaadd8cb00a725afa55bccbb8de7547dedac9350d79a9a470918245617e075c56a91adc36fb653bbe8a0a325d59b00443155a7e1a81ebf22e4e4cf56d9
fa066f1b66 test: Run feature_cltv with MiniWallet (MarcoFalke)
fa5591d38c test: Hide tx rehash in helper (MarcoFalke)
fa5f938cfe test: Remove new_tx reference (MarcoFalke)
Pull request description:
Allows to run the test even with no wallet compiled in
ACKs for top commit:
theStack:
ACK fa066f1b66💽
Tree-SHA512: 3f659a178ba3ee0baffd70fddf8b8a68e5551d85626c7f254b234d7f75e6a16430a32a7952037db358b579f045b4d296b46156f72e5d226f3e80334dc635ca10
ebd4be43cc doc: add release notes for 20867 (Antoine Poinsot)
5aa50ab9cc rpc/util: multisig: only check redeemScript size is <= 520 for P2SH (Antoine Poinsot)
063df9e897 test/functional: standardness sanity checks for P2(W)SH multisig (Antoine Poinsot)
ae0429d3af script: allow up to 20 keys in wsh() descriptors (Antoine Poinsot)
9fc68faf35 script: match multisigs with up to MAX_PUBKEYS_PER_MULTISIG keys (Antoine Poinsot)
Pull request description:
As described in https://github.com/bitcoin/bitcoin/issues/20620 multisigs are currently limited to 16 keys in descriptors and RPC helpers, even for P2WSH and P2SH-P2WSH.
This adds support for multisig with up to 20 keys (which are already standard) for Segwit v0 context for descriptors (`wsh()`, `sh(wsh())`) and RPC helpers.
Fixes https://github.com/bitcoin/bitcoin/issues/20620
ACKs for top commit:
meshcollider:
re-utACK ebd4be43cc
instagibbs:
re-ACK ebd4be43cc
Tree-SHA512: 36141f10a8288010d17d5c4fe8d24878bcd4533b88a8aba3a44fa8f74ceb3182d70fee01427e0ab7f53ce7fab46c88c1cd3ac3b18ab8a10bd4a6b8b74ed79e46
c5bb142817 test: resolve bug in test/functional/interface_bitcoin_cli.py - Test -getinfo with -rpcwallet=unloaded wallet returns no balances (klementtan)
Pull request description:
I think there is a bug in this test case where the new value of `cli_get_info` is not asserted.
ACKs for top commit:
jonatack:
ACK c5bb142817
Tree-SHA512: 50c0c2c8fe63c95f951dee892fbacedf92208f47efe5ed481fbb255f15137c799d9200fa3ff31a442df0691248d7ff04d899842722c3032cd7f35553622ba38c
Allow RPC users to opt-in to unsafe inputs when funding a raw transaction.
Applications that need to manage a complex RBF flow (such as lightning
nodes using anchor outputs) are very limited if they can only use safe inputs.
Fixes#21299
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d
jonatack:
Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
Replaces the existing tests in the test/lint/lint-filenames.sh and test/lint/lint-shebang.sh linter tests, as well as adding some new and increased testing.
Summary of tests:
- Checks every file in the repository against an allowed regexp to make sure only lowercase or uppercase alphanumerics (a-zA-Z0-9), underscores (_), hyphens (-), at (@) and dots (.) are used in repository filenames.
- Checks only source files (*.cpp, *.h, *.py, *.sh) against a stricter allowed regexp to make sure only lowercase alphanumerics (a-z0-9), underscores (_), hyphens (-) and dots (.) are used in source code filenames. Additionally there is an exception regexp for directories or files which are excepted from matching this regexp (This should replicate the existing test/lint/lint-filenames.sh test)
- Checks all files in the repository match an allowed executable or non-executable file permission octal. Additionally checks that for executable files, the file contains a shebang line.
- Checks that for executable .py and .sh files, the shebang line used matches an allowable list of shebangs (This should replicate the existing test/lint/lint-shebang.sh test)
- Checks every file that contains a shebang line to ensure it has an executable permission
Fixes#21729
a33bdb52d1 [tests] Speed up p2p_segwit.py (John Newbery)
Pull request description:
Never sleep for more than 5 seconds when waiting for an
inv-getdata exchange to time out.
Shaves about 1 minute of the runtime of p2p_segwit.py.
ACKs for top commit:
MarcoFalke:
review ACK a33bdb52d1🐳
Tree-SHA512: 7bd892ed0b1b817579f88910ba4714519bd0d871241e1b9a67968d297de1ed63d558115abad2aae4d105ff176c35a7079a3a789f3053442aed30d6e1aefb5c4a
fa40eb5b6b test: Speed up mempool_spend_coinbase.py (MarcoFalke)
fa29382ab2 test: Fix test cache issue (MarcoFalke)
fa085b470a test: Create MiniWallet.create_self_transfer (MarcoFalke)
fa1bedb494 test: Add MiniWallet.sendrawtransaction (MarcoFalke)
Pull request description:
Locally the test will run 4 seconds faster with `--valgrind` (18s vs 14s)
ACKs for top commit:
mjdietzx:
crACK fa40eb5b6b
Tree-SHA512: ecfb60dda5ca5d7e6367bb9c6210390d95ebf6396ce657728901d118b75bb90c98f9351df3b01004d00682234448d6c6a13338d12097f7dced2cf7f1bd84d924
84934bf70e multiprocess: Add echoipc RPC method and test (Russell Yanofsky)
7d76cf667e multiprocess: Add comments and documentation (Russell Yanofsky)
ddf7ecc8df multiprocess: Add bitcoin-node process spawning support (Russell Yanofsky)
10afdf0280 multiprocess: Add Ipc interface implementation (Russell Yanofsky)
745c9cebd5 multiprocess: Add Ipc and Init interface definitions (Russell Yanofsky)
5d62d7f6cd Update libmultiprocess library (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This PR adds basic process spawning and IPC method call support to `bitcoin-node` executables built with `--enable-multiprocess`[*].
These changes are used in https://github.com/bitcoin/bitcoin/pull/10102 to let node, gui, and wallet functionality run in different processes, and extended in https://github.com/bitcoin/bitcoin/pull/19460 and https://github.com/bitcoin/bitcoin/pull/19461 after that to allow gui and wallet processes to be started and stopped independently and connect to the node over a socket.
These changes can also be used to implement new functionality outside the `bitcoin-node` process like external indexes or pluggable transports (https://github.com/bitcoin/bitcoin/pull/18988). The `Ipc::spawnProcess` and `Ipc::serveProcess` methods added here are entry points for spawning a child process and serving a parent process, and being able to make bidirectional, multithreaded method calls between the processes. A simple example of this is implemented in commit "Add echoipc RPC method and test."
Changes in this PR aside from the echo test were originally part of #10102, but have been split and moved here for easier review, and so they can be used for other applications like external plugins.
Additional notes about this PR can be found at https://bitcoincore.reviews/19160
[*] Note: the `--enable-multiprocess` feature is still experimental, and not enabled by default, and not yet supported on windows. More information can be found in [doc/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md)
ACKs for top commit:
fjahr:
re-ACK 84934bf70e
ariard:
ACK 84934bf. Changes since last ACK fixes the silent merge conflict about `EnsureAnyNodeContext()`. Rebuilt and checked again debug command `echoipc`.
Tree-SHA512: 52a948b5e18a26d7d7a09b83003eaae9b1ed2981978c36c959fe9a55abf70ae6a627c4ff913a3428be17400a3dace30c58b5057fa75c319662c3be98f19810c6
d831e711ca [validation] RewindBlockIndex no longer needed (Dhruv Mehta)
Pull request description:
Closes#17862
Context from [original comment](https://github.com/bitcoin/bitcoin/issues/17862#issuecomment-744285188) (minor edits):
`RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows:
- A pre-segwit (i.e. v0.13.0 or older) node is running.
- Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules.
- The user upgrades the node to a segwit-aware version (v0.13.1 or newer).
- On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules.
- those blocks are then redownloaded (with witness data) and validated with segwit rules.
This logic probably isn't required any more since:
- Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested.
- Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22.
This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data.
Removing this code allows the following (done in follow-up #21090):
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
jnewbery:
utACK d831e711ca
jamesob:
ACK d831e711ca
laanwj:
Cursory code review ACK d831e711ca. Agree with the direction of the change, thanks for simplifying the logic here.
glozow:
utACK d831e711ca
Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053
This value is no longer used and is instead specified statically
in chainparams. This change means that previously generated
snapshots will no longer be usable.
a732ee353c [test] Add tests for addr relay in -blocksonly mode (Amiti Uttarwar)
a6694eaed8 [test] Add address relay tests involving outbound peers (Martin Zumsande)
8188b77c17 [test] Add tests for getaddr behavior (Martin Zumsande)
d2dbfe6ff1 [test] Extract sending an addr message into a helper (Amiti Uttarwar)
c991943399 [test] Refactor the addr relay test to prepare for new tests (Amiti Uttarwar)
Pull request description:
This extends the functional test `p2p_addr_relay.py`.
It adds test coverage for address relay involving outbound peers, tests for both outgoing and incoming `GETADDR` requests and tests for `-blocksonly` mode.
The initial refactors and some of the new tests were taken from Amiti Uttarwar's PR #21528 - they are general test improvements not directly tied to the change proposed there.
ACKs for top commit:
amitiuttarwar:
re-ACK a732ee353c, small diff based on code review
MarcoFalke:
Concept ACK a732ee353c🌊
Tree-SHA512: e80d52683808ddd6b948a5134239f002f3fecf61b60e187877b07be6251721fde847104e495c75a1a5133a09c0b41a9255a0bec82932c0b304b516fa89bce33e
Add simple interfaces::Echo IPC interface with one method that just takes and
returns a string, to test multiprocess framework and provide an example of how
it can be used to spawn and call between processes.
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
fadcd3f78e doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad2 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3 move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a1 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f refactor: Move load block thread into ChainstateManager (MarcoFalke)
Pull request description:
This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
* Breaking down the massive files init.cpp and validation.cpp into logical units
* Creating a standalone-module to reduce the mental complexity
* Pave the way to fix validation related circular dependencies
* Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
ACKs for top commit:
promag:
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
jamesob:
ACK fadcd3f78e ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
ryanofsky:
Code review ACK fadcd3f78e. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
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.
