f680d27155 test: use MiniWallet for make_utxo helper in feature_rbf.py (Sebastian Falbesoner)
0f27524602 test: scale amounts in test_doublespend_tree down by factor 10 (Sebastian Falbesoner)
d1e2481274 test: scale amounts in test_doublespend_chain down by factor 10 (Sebastian Falbesoner)
Pull request description:
This PR aims to further increase MiniWallet usage in the functional test feature_rbf.py by using it in the `make_utxo(...)` helper, which is the only part that needs a wallet for most sub-tests. In order to do that, the amounts for the utxos have to be scaled down in two sub-tests first (`test_doublespend_chain` and `test_doublespend_tree`, see first two commits), since we need amounts passed to `make_utxo` than can be funded by only one input. For creating UTXOs with a value of 50 BTC, we'd need to implement a method for consolidating multiple utxos into one first, which seems to be overkill.
Note that after this PR's change, there is only one sub-test left (`test_rpc`) that needs the wallet compiled into bitcoind.
ACKs for top commit:
MarcoFalke:
review ACK f680d27155🦐
Tree-SHA512: 46c8c245086a9e79855c4ede2f8f412333cf2658136805196b203b3567c89398d77fcb80715c0bb72fdc84331cc67544b2fdc259193a3adcb2fc36e147c26fce
357f0c7233 ci: Enable more functional tests on Windows MSVC task (Hennadii Stepanov)
f55932678f qa: Fix "RuntimeError: Event loop is closed" on Windows (Hennadii Stepanov)
Pull request description:
On master (2161a05855), running functional tests that use the P2P interface ends with an error:
```
RuntimeError: Event loop is closed
```
This PR fixes this bug, and enables more functional tests on Windows MSVC CI task.
More details about bugfix:
- [What’s New In Python 3.7](https://docs.python.org/3/whatsnew/3.7.html#asyncio)
- https://bugs.python.org/issue33792
- actual [change](https://docs.python.org/3.8/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy) done in Python 3.8
Excluded tests, that are listed in the `EXCLUDE_TESTS` environment variable, need more thorough investigation to be enabled.
ACKs for top commit:
MarcoFalke:
review ACK 357f0c7233🌆
Tree-SHA512: d0ba85be81d55c934959ce7402a9c726598125e9751a1de179d16759d0e8b8a915de879c3a62c12d3564c5e0d9649ebd86963744449626efaa42d9eaa99ad3d0
fa66a7d732 p2p: Rename fBlocksOnly, Add test (MarcoFalke)
fac66d0a39 test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method (MarcoFalke)
Pull request description:
`fBlocksOnly` has several issues:
* The name is confusing
* It is untested
Fix both.
ACKs for top commit:
laanwj:
Code review ACK fa66a7d732
Tree-SHA512: 4218f455eeb37297f74603d7d44895288605844ae828a40dfb7a70215f1a058ac5ad945a22732f5ebcad3ad375d54ba360bea69ea79639a30d4c88b042448f0f
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
fab2e23b57 Use generate* from TestFramework (MarcoFalke)
faf7e92804 test: Add generate* calls to test framework (MarcoFalke)
Pull request description:
This is needed for #22567.
By making the calls to `generate*` member function of the test framework, it paves the way to make it easier to implicitly call the `sync_all` member function.
ACKs for top commit:
jnewbery:
utACK fab2e23b57
Tree-SHA512: 7a7be6be71f0602119689df45d63a1adec309f323eac2330ee0f200676001afe825605859bd02c6a8a8dcf85d925dc1bc37370ef1ceb8ad1d85a66eec0dbfff9
021daedfa1 refactor: replace remaining binascii method calls (Zero-1729)
Pull request description:
This PR removes the remaining `binascii` method calls outside `test/functional` and `test_framework`, as pointed out here https://github.com/bitcoin/bitcoin/pull/22619#pullrequestreview-722153458.
Follow-up to #22593 and #22619Closes#22605
ACKs for top commit:
josibake:
re-ACK 021daedfa1
theStack:
re-ACK 021daedfa1
Tree-SHA512: 2ae9fee8917112c91a5406f219ca70f24cd8902b903db5a61fc2de85ad640d669a772f5c05970be0fcee6ef1cdd32fae2ca5d1ec6dc9798b43352c8160ddde6f
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
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
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
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
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.
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
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.
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
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
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.
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
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
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
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.
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
- 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
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
fa066f1b66 test: Run feature_cltv with MiniWallet (MarcoFalke)
fa5591d38c test: Hide tx rehash in helper (MarcoFalke)
fa5f938cfe test: Remove new_tx reference (MarcoFalke)
Pull request description:
Allows to run the test even with no wallet compiled in
ACKs for top commit:
theStack:
ACK fa066f1b66💽
Tree-SHA512: 3f659a178ba3ee0baffd70fddf8b8a68e5551d85626c7f254b234d7f75e6a16430a32a7952037db358b579f045b4d296b46156f72e5d226f3e80334dc635ca10
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d
jonatack:
Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
fa40eb5b6b test: Speed up mempool_spend_coinbase.py (MarcoFalke)
fa29382ab2 test: Fix test cache issue (MarcoFalke)
fa085b470a test: Create MiniWallet.create_self_transfer (MarcoFalke)
fa1bedb494 test: Add MiniWallet.sendrawtransaction (MarcoFalke)
Pull request description:
Locally the test will run 4 seconds faster with `--valgrind` (18s vs 14s)
ACKs for top commit:
mjdietzx:
crACK fa40eb5b6b
Tree-SHA512: ecfb60dda5ca5d7e6367bb9c6210390d95ebf6396ce657728901d118b75bb90c98f9351df3b01004d00682234448d6c6a13338d12097f7dced2cf7f1bd84d924
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.
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
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
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
