fafe896a0b test: Set regtest.BIP66Height = 102 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 66. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 66, which is enforced on mainnet for all new blocks.
ACKs for top commit:
GeneFerneau:
Concept + code review ACK [fafe896](fafe896a0b)
0xB10C:
crACK fafe896a0b
laanwj:
ACK fafe896a0b
Zero-1729:
tACK fafe896
kristapsk:
ACK fafe896a0b. Full functional test suite showed few second speed incrase on my laptop (although I didn't do proper benchmarking with multiple runs, just single `time ./test/functional/test_runner.py` on current master vs this PR).
theStack:
Tested ACK fafe896a0b
hg333:
tACK fafe896a0b
Tree-SHA512: 4bbee3c8587d612e74a59fde49b6439c1296f2fc27d3a7cf59a35e920f729fdd581c930290bd04def618f81412236676ddb99b4ceb4d80dfb9fd610b128a04b1
accf3d5868 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7 [test] helper function to increase transaction weight (glozow)
f8253d69d6 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d3 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa13 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aa misc package validation doc improvements (glozow)
Pull request description:
This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).
Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.
#### Motivation
It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:
- We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
- We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.
ACKs for top commit:
JeremyRubin:
utACK accf3d5
ariard:
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
bb822a7af8 wallet, rpc: add listdescriptors private option (S3RK)
Pull request description:
Rationale: make it possible to backup your wallet with `listdescriptors` command
* The default behaviour is still to show public version
* For private version only the root xprv is returned
Example use-case:
```
> bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
> bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt
> bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
> bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
```
In case of watch-only wallet without private keys there will be following output:
```
error code: -4
error message:
Can't get descriptor string.
```
ACKs for top commit:
achow101:
re-ACK bb822a7af8
Rspigler:
tACK bb822a7af8
jonatack:
ACK bb822a7af8 per `git diff 2854ddc bb822a7`
prayank23:
tACK bb822a7af8
meshcollider:
Code review ACK bb822a7af8
Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
5a1bef60a0 test: refactor: remove binascii from test_framework (Zero-1729)
Pull request description:
This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`, and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.
Additionally, certain changes made are based on the following assumption:
```
bytes.hex(data) == binascii.hexlify(data).decode()
bytes.hex(data).encode() == binascii.hexlify(data)
```
Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.
closes#22605
ACKs for top commit:
theStack:
Code-review ACK 5a1bef60a0🔢
Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
222290f543 test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be test: Create all blocks with version 4 or higher (MarcoFalke)
Pull request description:
BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.
I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)
This pull is done in two commits:
* test: Create all blocks with version 4 or higher:
Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.
* test: Set BIP34Height = 2 for regtest:
This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed
ACKs for top commit:
ajtowns:
ACK 222290f543
jonatack:
ACK 222290f543 tested and reviewed rebased to current master 5e213822f8
theStack:
Tested ACK 222290f543
Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
82b6f89819 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64 [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed56 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bd [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2 [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a9 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c08719778 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)
Pull request description:
This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).
The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.
The tests include:
* parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
* the delay before querying DNS seeds depending on how many addresses are in the addrman
* the behavior of `-forcednsseed`
* skipping DNS querying if we have outbound full relay connections & not block-relay-only connections
Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽
ACKs for top commit:
mzumsande:
ACK 82b6f89819
jnewbery:
reACK 82b6f89819
Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
3f7250b328 [test] Use the new endpoint to improve tests (Amiti Uttarwar)
3893da06db [RPC] Add field to getpeerinfo to indicate if addr relay is enabled (Amiti Uttarwar)
0980ca78cd [test] Test that we intentionally select addr relay peers. (Amiti Uttarwar)
c061599e40 [net_processing] Remove RelayAddrsWithPeer function (Amiti Uttarwar)
201e496481 [net_processing] Introduce new field to indicate if addr relay is enabled (Amiti Uttarwar)
1d1ef2db7e [net_processing] Defer initializing m_addr_known (Amiti Uttarwar)
6653fa3328 [test] Update p2p_addr_relay test to prepare (Amiti Uttarwar)
2fcaec7bbb [net_processing] Introduce SetupAddressRelay (Amiti Uttarwar)
Pull request description:
This PR builds on the test refactors extracted into #22306 (first 5 commits).
This PR aims to reduce addr blackholes. When we receive an `addr` message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn't relay would effectively "blackhole" the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.
This implementation defers initialization of `m_addr_known` until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their `version` message. For inbound peers, we initialize the filter if/when we get an addr related message (`ADDR`, `ADDRV2`, `GETADDR`). We do NOT initialize the filter based on a `SENDADDRV2` message.
To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:
- Posted to the [mailing list](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018784.html)
- Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in https://github.com/bitcoin/bitcoin/pull/21528#issuecomment-809906430. Many have confirmed that this change would not be problematic.
- Raised as topic during [bitcoin-core-dev meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-03-25.html#l-954)
- Raised as topic during [bitcoin p2p meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-04-20.html#l-439)
ACKs for top commit:
jnewbery:
reACK 3f7250b328
glozow:
ACK 3f7250b328
ajtowns:
utACK 3f7250b328
Tree-SHA512: 29069282af684c1cd37d107c395fdd432dcccb11626f3c2dabfe92fdc4c85e74c7c4056fbdfa88017fec240506639b72ac6c311f8ce7c583112eb15f47e421af
607076d01b test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c74ed test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe133 test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)
Pull request description:
This is a very late follow-up PR to #10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also #10608 in why it was considered confusing and superfluous).
Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.
ACKs for top commit:
MarcoFalke:
review ACK 607076d01b🚴
Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
8a2b58db9e test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)
Pull request description:
This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program) is defined as follows:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". **The following byte vector pushed is called the "witness program".**
In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](https://github.com/bitcoin/bitcoin/pull/22363#discussion_r666794261)!
Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f31ee, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
This was fixed in PR https://github.com/bitcoin/bips/pull/416 later.
So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.
ACKs for top commit:
josibake:
tACK 8a2b58db9e
Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
When starting up with a populated addrman, ThreadDNSAddressSeed adds a delay
during which time the node may be able to connect to some peers. This commit
tests the delay changes based on the number of addresses in the addrman.
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
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.
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
W. J. van der Laan