7daffc6a90a797ce7c365a893a31a31b0206985c [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90a797ce7c365a893a31a31b0206985c
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
Migrates the CScriptNum decode tests into a unit test, and moved some
changes made in #14816. Made possible by the integration of
test_framework unit testing in #18576. Further extends the original
test with larger ints, similar to the scriptnum_tests.cpp file. Adds
test to blocktools.py testing fn create_coinbase() with CScriptNum
decode.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
Mypy is used in lint-python.sh to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
Useful resources:
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
23083856a551ca13e8b142791c296ecb25cc4e7f [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25ac4039056808affeb5ffa86a2c317fe [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e94571fe984857494042ac292c17156 [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f
jkczyz:
re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5 🌳
theStack:
ACK 23083856a5
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e08a04807c71eb02104ca9589eaadf1b [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c851e25e1a533bf1d4296f6b55c81332 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142246b5d51056a51dbf4645b31802be4 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
de8905adf204c42bba810802f82b98f7b3dd26dc test: use unittest and test_runner for test framework unit testing (Gloria Zhao)
Pull request description:
Proposal for unit testing on test_framework functions:
1. Use the python `unittest` library. Don't use test_framework to test itself.
2. Put the tests inside the same file as the functions they are testing.
3. Call the tests from `test_runner.py`. To include more Test Framework tests, add the filename to the list `TEST_FRAMEWORK_MODULES`. Don't add new files or change the list of accepted script prefixes.
Makes these changes for `bn2vch` (followup to [this comment](https://github.com/bitcoin/bitcoin/pull/18378#pullrequestreview-377271264)).
ACKs for top commit:
jnewbery:
Tested ACK de8905adf204c42bba810802f82b98f7b3dd26dc. Great stuff gzhao408 . Thanks for this!
Tree-SHA512: 91572d43e203a1864765b93a9472667994115ec38b271f2b2f9fcd0f0112b393fc24ba7d2371d5a34b0a6a4522f6b934fc5164363819aa7ed8d6c6c9a60cc101
8098dea06944f9de8b285f44958eb98761f133ee test: Add mempool_updatefromblock.py (Hennadii Stepanov)
Pull request description:
This PR adds a new test for mempool update of transaction descendants/ancestors information (count, size) when transactions have been re-added from a disconnected block to the mempool.
It could be helpful for working on PRs like #17925, #18191.
ACKs for top commit:
ariard:
ACK 8098dea
Tree-SHA512: 7e808fa8df8d7d7a7dbdc3f79361049b49c7bce9b58fd5539b28c9636bedac747695537e500d7ed68dc8bdb80167ad3f1c01086f7551691405d2ba2e38ef1d06
66fe7b1a98c03f690dcf60d359baac124658aeae test: added test for upgradewallet RPC (Harris)
Pull request description:
This PR adds tests for the newly merged *upgradewallet* RPC.
Additionally, it expands `test_framework/util.py` by adding the function `adjust_bitcoin_conf_for_pre_17` to support nodes that don't parse configuration sections.
This test uses two older node versions, v0.15.2 and v0.16.3, to create older wallet versions to be used by `upgradewallet`.
Fixes https://github.com/bitcoin/bitcoin/issues/18767
Top commit has no ACKs.
Tree-SHA512: bb72ff1e829e2c3954386cc308842820ef0828a4fbb754202b225a8748f92d4dcc5ec77fb146bfd5484a5c2f29ce95adf9f3fb4483437088ff3ea4a8d2c442c1
50fc4df6c4e8a84bdda13ade7bed7a2131796f00 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a1785360c4db662a7f3d3ade7b6b503258d39 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472d3625416f0e7796e9f2a0379d14d49 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5e5526215561311c184a8cbc345ecdc [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a3aa39651fbc1f9fa3df1a49f1f5868 [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3bc5a1b7bb0284e06f9e2e69454f5ba [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a3335f8e871cc3f5286af4546dff66172a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
MarcoFalke:
ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
sipa:
utACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
Test the test_framework, but don't use test_framework objects or functions to test itself
Use python unittest library and put test_framework's unit tests inside their respective files
Add the filename to TEST_FRAMEWORK_MODULES in test_runner
Aggregate all test_framework tests into one TestSuite to run before the functional tests in test_runner
Delete framework_test_script, move test_bn2vch to script.py and add to TEST_FRAMEWORK_MODULES in test_runner
Check that...
- mempool tracks & reattempts delivery of a transaction where a GETDATA hasn't
been requested by a peer yet.
- transaction delivery is not attempted again after GETDATA is received.
- transaction is removed from the unbroadcast set when its removed from the
mempool.
Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.
Some tests are modified to work with --descriptors and run with that
option in test_runer:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py
* wallet_keypool_topup.py
* wallet_labels.py
* wallet_avoidreuse.py
7524b6479cb20471d827aec5500925c86c62ce1c Add tests for generateblock (Andrew Toth)
dcc8332543f8fb6d1bb47cb270fcbb6a814a7d6e Add generateblock rpc (Andrew Toth)
Pull request description:
The existing block generation rpcs for regtest, `generatetoaddress` and `generatetodescriptor`, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example:
- Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead.
- Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined.
- Testing for non-standard transactions that are mined.
- Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block.
This PR introduces a new rpc, `generateblock`, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor.
This reopens#17653 since it was closed by mistake.
Thanks to instagibbs for code suggestions that I used here.
ACKs for top commit:
MarcoFalke:
re-ACK 7524b6479cb20471d827aec5500925c86c62ce1c 📁
Tree-SHA512: 857106007465b5b9b8a84b6d07c17cbf8378a33a72d32ff79abea1d5ab4babb4d53a11ddbb14595aa1fac9dfa1391e3a11403d742f69951beea2f683e8a01cd4
fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde test: Add basic addr relay test (MarcoFalke)
fa1793c1c44a3f75a09f9c636467b8274c541bdd net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003f53708b6d5df1ed4e7f8a7c68aa3ac net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
fa156999695ddaeb016d8320bee62f8d96679d55 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa156999695ddaeb016d8320bee62f8d96679d55 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
c3857c5fcb21836ddc1b79a6b19cffe562cade10 wallet: remove CreateTotalBumpTransaction() (Jon Atack)
4a0b27bb01738e6917e27b2cf47f9a8536249693 wallet: remove totalfee from createBumpTransaction() (Jon Atack)
e347cfa9a7244277f9d220a4dc3537182f18441e rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack)
bd05f96d79df1a1561f84850d777808f8575fb8b test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack)
a6d1ab8caa63bd343207baa60edb705209f16fb4 test: update bumpfee testing from totalFee to fee_rate (Jon Atack)
Pull request description:
Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it.
ACKs for top commit:
laanwj:
ACK c3857c5fcb21836ddc1b79a6b19cffe562cade10
Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00
111880aaf7e12a12f0797f1b19673e3d96328edd [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley)
Pull request description:
This adds light functional coverage to estimaterawfee - a subset of
the testing applied to estimatesmartfee, and argument validation
testing to both estimaterawfee and estimatesmartfee.
One valid estimatesmartfee signature test is commented out because it
fails currently.
Extracted from #12940
Top commit has no ACKs.
Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
to verify node behaviour and debug log when launching bitcoind in these cases:
1. `bitcoind` with no -asmap arg, using /16 prefix for IP bucketing
2. `bitcoind -asmap=<relative path>`, using the unit test skeleton asmap
3. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
4. `bitcoind -asmap` with no file specified, and a missing default asmap file
The tests are order-independent. The slowest test (missing default asmap file)
is placed last.
This tests that a mempool transaction expires after a given timeout
and its children are removed as well.
Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
a user definable expiry timeout via the -mempoolexpiry=<n> command
line argument (<n> is the timeout in hours) are tested.
92b2f5306ba0b3f031293cb8f415b67cb002c2f1 test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3ddef931896a7e9dcfa6704e305a69fbff devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c9918d10c69a46e6ceb3cb1a5e04edf5bc rpc: add dumptxoutset (James O'Beirne)
92fafb3a7da66f737e960e541fcfbcadedf6043a coinstats: add coins_count (James O'Beirne)
707fde7b9ba522c22179e2db0ed7b462c65138d9 add unused SnapshotMetadata class (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.
All of this is unused at the moment.
ACKs for top commit:
laanwj:
ACK 92b2f5306ba0b3f031293cb8f415b67cb002c2f1
Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
a6f6f77a86a50de32275f7aac37aa6eaf79f79eb QA: Add wallet_implicitsegwit to test the ability to transform keys between address types (Luke Dashjr)
Pull request description:
This makes sure the wallet recognises payments to keys via address types they weren't created with.
While we don't *want* this behaviour, it might make sense to explicitly test that it works until we remove it.
ACKs for top commit:
adamjonas:
utACK a6f6f77a86a50de32275f7aac37aa6eaf79f79eb
Tree-SHA512: b208405729277e9ce06eb772b45e8d1683c4dc5703754448b8f19a590b37522abd7bb46d4dbd41513b3d46d7f9e8769ce4f15fa4114be600f31a1ebbc1157840
This adds light functional coverage to estimaterawfee - a subset of
the testing applied to estimatesmartfee, and argument validation
testing to both estimaterawfee and estimatesmartfee.
One valid estimatesmartfee signature test is commented out because it
fails currently.
333317ce6b67aa92f7363d48cd750712190b4b6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4ee471c4641eef8de02702ba0619ae7 test: Pass down correct chain name in tests (MarcoFalke)
Pull request description:
To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them
ACKs for top commit:
Sjors:
Thanks for adding the node 1 example. Code review ACK 333317c
Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
d117f4541d4717e83c9396273e92960723622030 Add test for setban (nicolas.dorier)
dc7529abf0197dccb876dc4a93cbdd2ad9f03e5c [Fix] Allow connection of a noban banned peer (nicolas.dorier)
Pull request description:
Reported by @MarcoFalke on https://github.com/bitcoin/bitcoin/pull/16248#discussion_r314026195
The bug would mean that if the peer connecting to you is banned, but whitelisted without specific permissions, it would not be able to connect to the node.
The solution is just to move the same line below.
ACKs for top commit:
Sjors:
Agree inline is more clear. utACK d117f45
MarcoFalke:
ACK d117f4541d4717e83c9396273e92960723622030
Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811
