faf45d1f1f997c316fc4c611a23c4456533eefe9 http: Avoid crash when g_thread_http was never started (MarcoFalke)
fa12a37b27f0570a551b8c103ea6537ee4a8e399 test: Replace inline-comments with logs, pep8 formatting (MarcoFalke)
fa83b39ff3ae3fbad93df002915c0e5f99c104a9 init: Remove confusing and redundant InitError (MarcoFalke)
Pull request description:
Avoid a crash during shutdown when the init sequence failed for some reason
ACKs for top commit:
promag:
Tested ACK faf45d1f1f997c316fc4c611a23c4456533eefe9.
ryanofsky:
Code review ACK faf45d1f1f997c316fc4c611a23c4456533eefe9. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test
hebasto:
ACK faf45d1f1f997c316fc4c611a23c4456533eefe9, tested on Linux Mint 19.3 with the following patch:
Tree-SHA512: 59632bf01c999e65c724e2728ac103250ccd8b0b16fac19d3a2a82639ab73e4f2efb86c78e63c588a5954625d8d0cf9545e2a7e070e6e15d2a54beeb50e00b61
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
fa03713e133e3017112fdd5c278e0c8643054578 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) (MarcoFalke)
Pull request description:
actually (?) fix#18561
See most recent traceback https://travis-ci.org/github/bitcoin/bitcoin/jobs/674668692#L7062
I believe the reason the error is still there is that ConnectionResetError is derived from OSError:
ConnectionResetError(ConnectionError(OSError))
And IOError is an alias for OSError since python 3.3, see https://docs.python.org/3/library/exceptions.html#IOError
So fix that by renaming IOError to the alias OSError and move the less specific catch clause down a few lines.
ACKs for top commit:
jonatack:
ACK fa03713e133e3017112fdd5c278e0c8643054578
Tree-SHA512: 6e5b214ed9101bf8ebe7472dcc1f9e9d128e2575c93ec00c8d0774ae1a9b52a8c2a653a45a0eab8d881570b08dd5ffeddf5aca88a10438c366e1f633253cb0b5
38677274f931088218eeb1f258077d3387f39c89 rpc: settxfee respects -maxtxfee wallet setting (Fabian Jahr)
bda84a08a0ac92dff6cadc99cf9bb8c3fadd7e13 rpc: Add documentation for deactivating settxfee (Fabian Jahr)
Pull request description:
~~Closes 18315~~
`settxfee` can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that `settxfee` is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate.
Examples:
```
$ src/bitcoin-cli settxfee 0.0000000
{
"active": false,
"fee_rate": "0.00000000 BTC/kB"
}
$ src/bitcoin-cli settxfee 0.0001
{
"active": true,
"fee_rate": "0.00010000 BTC/kB"
}
```
ACKs for top commit:
MarcoFalke:
ACK 38677274f931088218eeb1f258077d3387f39c89, seems useful to error out early instead of later #16257🕍
jonatack:
ACK 38677274f931088218eeb
meshcollider:
LGTM, utACK 38677274f931088218eeb1f258077d3387f39c89
Tree-SHA512: 642813b5cf6612abb4b6cb63728081a6bd1659d809e0149c8f56060b6da7253fee989b3b202854f3051df3773c966799af30b612648c466b099f00590f356548
cc84460c164bcb2a874d4f08b3a2624e5ee9ff0a test: move sync_blocks and sync_mempool functions to test_framework.py (Roy Shao)
Pull request description:
This PR moves `sync_blocks` and `sync_mempool` out from `test_framework/util.py` to `test_framework/test_framework.py` so they can take contextual information of test framework into account.
* Change all reference callers to call functions from `test_framework.py`
* Remove `**kwargs` which is not used
* Take into account of `timeout_factor` when respecting timeout in function implementations.
* Pass all tests by running `./test/functional/test_runner.py`
fixes#18930
ACKs for top commit:
MarcoFalke:
ACK cc84460c164bcb2a874d4f08b3a2624e5ee9ff0a , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫
Tree-SHA512: a79b2a3fa842fc26a7aacb834bb2aea88b3049916c0b754e60002a77ce94bb5954e0ea3b436bf268e9295efb62d721dfef263a09339a55c684ac3fda388c275e
fac3716b09bb9ee121db629873d9694a95cae942 test: check that peer is connected when calling sync_* (MarcoFalke)
Pull request description:
Without a connection there is no way to sync, so we can fail early and don't have to wait for the timeout
ACKs for top commit:
jonatack:
ACK fac3716b09bb9
Tree-SHA512: 12f771473c23e152dae4bfb201fadb2c3530cf439de64fea07d048734614543080a5d05c9c36e6e398c6a69c8279f609d34706599571814172a11bcfbea4a3b9
38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc docs: Add notes on how to diasble rpc timeout in functional tests while attatching gdb. (codeShark149)
784ae096259955ea7a294114f5c021c9489d2717 test: Add capability to disable RPC timeout in functional tests. (codeShark149)
Pull request description:
Many times, especially while debugging RPC callbacks to core using gdb, the test timeout kicks in before the response can get back. This can be annoying and requires restarting the functional test as well as gdb attachment.
This PR adds a `--notimeout` flag into `test_framework` and sets the `rpc_timeout` accordingly if the flag is set.
The same effect can be achieved with newly added `--factor` flag but keeping a separate flag that explicitly disables the timeout can be easier for new testers to find it out and separates its purpose from the `--factor` flag.
Requesting review ryanofsky jnewbery as per the IRC discussion.
Update: After initial round of review, the approach is modified to accommodate the functionality in already existing `--factor` flag. `--factor` is changed to `--timeout-factor` to express its intent better.
ACKs for top commit:
MarcoFalke:
ACK 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc and thanks for fixing up all my typos 😅
jnewbery:
ACK 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc.
Tree-SHA512: 9458dd1010288c62f8bb83f7a4893284fbbf938882dd65fc9e08810a910db07ef676e3100266028e5d4c8ce407b2267b3860595015da070c84a9d4a9816797db
2742c3428633b6ceaab6714635dc3adb74bf121b test: add factor option to adjust test timeouts (Harris)
Pull request description:
This PR adds a new option **factor** that can be used to adjust timeouts in various functional tests.
Several timeouts and functions from `authproxy`, `mininode`, `test_node` and `util` have been adapted to use this option. The factor-option definition is located in `test_framework.py`.
Fixes https://github.com/bitcoin/bitcoin/issues/18266
Also Fixes https://github.com/bitcoin/bitcoin/issues/18834
ACKs for top commit:
MarcoFalke:
Thanks! ACK 2742c3428633b6ceaab6714635dc3adb74bf121b
Tree-SHA512: 6d8421933ba2ac1b7db00b70cf2bc242d9842c48121c11aadc30b0985c4a174c86a127d6402d0cd73b993559d60d4f747872d21f9510cf4806e008349780d3ef
1abcecc40c518a98b7d17880657ec0247abdf125 Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón)
Pull request description:
Simply avoiding the hardcoded string in more places for consistency.
It can also allow for more easily reusing tests for other chains other than regtest.
Separated from #8994 .
Continues #16509 .
It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO.
ACKs for top commit:
Sjors:
re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files.
elichai:
Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125
ryanofsky:
Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125.
ryanofsky:
Code review ACK 1abcecc40c518a98b7d17880657ec0247abdf125
Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
19139ee034d20ebab1b91d3ac13a8eee70b59374 Add documentation for test_shell submodule (JamesC)
f5112369cf91451d2d0bf574a9bfdaea04696939 Add TestShell class (James Chiang)
5155602a636c323424f75272ccec38588b3d71cd Move argparse() to init() (JamesC)
2ab01462f48b2d4e0d03ba842c3af8851c67c6f1 Move assert num_nodes is set into main() (JamesC)
614c645643e86c4255b98c663c10f2c227158d4b Clear TestNode objects after shutdown (JamesC)
6f40820757d25ff1ccfdfcbdf2b45b8b65308010 Add closing and flushing of logging handlers (JamesC)
6b71241291a184c9ee197bf5f0c7e1414417a0a0 Refactor TestFramework main() into setup/shutdown (JamesC)
ede8b7608e115364b5bb12e7f39d662145733de6 Remove network_event_loop instance in close() (JamesC)
Pull request description:
This PR refactors BitcoinTestFramework to encapsulate setup and shutdown logic into dedicated methods, and adds a ~~TestWrapper~~ TestShell child class. This wrapper allows the underlying BitcoinTestFramework to run _between user inputs_ in a REPL environment, such as a Jupyter notebook or any interactive Python3 interpreter.
The ~~TestWrapper~~ TestShell is motivated by the opportunity to expose the test-framework as a prototyping and educational toolkit. Examples of code prototypes enabled by ~~TestWrapper~~ TestShell can be found in the Optech [Taproot/Schnorr](https://github.com/bitcoinops/taproot-workshop) workshop repository.
Usage example:
```
>>> import sys
>>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
```
```
>>> from test_framework.test_wrapper import TestShell
>>> test = TestShell()
>>> test.setup(num_nodes=2)
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
```
```
>>> test.nodes[0].generate(101)
>>> test.nodes[0].getblockchaininfo()["blocks"]
101
```
```
>>> test.shutdown()
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Cleaning up /path/to/bitcoin_func_test_XXXXXXX on exit
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
```
**Overview of changes to BitcoinTestFramework:**
- Code moved to `setup()/shutdown()` methods.
- Argument parsing logic encapsulated by `parse_args` method.
- Success state moved to `BitcoinTestFramework.success`.
_During Shutdown_
- `BitcoinTestFramework` logging handlers are flushed and removed.
- `BitcoinTestFrameowork.nodes` list is cleared.
- `NetworkThread.network_event_loop` is reset. (NetworkThread class).
**Behavioural changes:**
- Test parameters can now also be set when overriding BitcoinTestFramework.setup() in addition to overriding `set_test_params` method.
- Potential exceptions raised in BitcoinTestFramework.setup() will be handled in main().
**Added files:**
- ~~test_wrapper.py~~ `test_shell.py`
- ~~test-wrapper.md~~ `test-shell.md`
ACKs for top commit:
jamesob:
ACK 19139ee034
jonatack:
ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374
jnewbery:
Rather than invalidate the three ACKs for a minor nit, can you force push back to 19139ee034d20ebab1b91d3ac13a8eee70b59374 please? I think this PR was ready to merge before your last force push.
jachiang:
> Rather than invalidate the three ACKs for a minor nit, can you force push back to [19139ee](19139ee034) please? I think this PR was ready to merge before your last force push.
jnewbery:
ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374
Tree-SHA512: 0c24f405f295a8580a9c8f1b9e0182b5d753eb08cc331424616dd50a062fb773d3719db4d08943365b1f42ccb965cc363b4bcc5beae27ac90b3460b349ed46b2
fa47330397 test: Speed up cache creation (MarcoFalke)
fa6ad7a5ec test: Bump MAX_NODES to 12 (MarcoFalke)
Pull request description:
When testing a combination of settings that affect the datadir (e.g. prune, blockfilter, ...) we may need a lot of datadirs.
Bump the maximum number of nodes proactively from 8 to 12, so that caches get populated with 12 node dirs, as opposed to 8.
Also, add an assert that the list of deterministic keys is exactly the number of max nodes (and not more than that.
Also, create the cache faster.
ACKs for commit fa4733:
laanwj:
utACK fa473303972b7dad600d949dc9b303d8136cb7e7
Tree-SHA512: 9803c765ed52d344102f5a3bce57b05d88a7429dcb05ed66ed6c881fda8d87c2834d02d21b95fe9f39c0efe3b8527e13cf94f006588cde22e8c2cd50b2d517a6
fa2cdc9ac2 test: Simplify create_cache (MarcoFalke)
fa25210d62 qa: Fix wallet_txn_doublespend issue (MarcoFalke)
1111aecbb5 qa: Always refresh stale cache to be out of ibd (MarcoFalke)
fab0d85802 qa: Remove mocktime unless required (MarcoFalke)
Pull request description:
When starting a test, we are always in IBD because the timestamps on cached blocks are in the past. Usually, we solve that by generating a block at the beginning of the test.
That is clumsy and might even lead to other problems such as #15360 and https://github.com/bitcoin/bitcoin/issues/14446#issuecomment-461926598
So fix that by getting rid of mocktime and always refreshing the last block of the cache when starting the test framework.
Should fix#14446
Tree-SHA512: 6af09800f9c86131349a103af617a54551f5f3f3260d38e14e3f30fdd3d91a0feb0100c56cbb12eae4aeac5571ae4b530b16345cbb831d2670237b53351a22c1
fa48405ef84985e5a9d38ec38e90d16596ea45b5 Warn on unknown rw_settings (MarcoFalke)
Pull request description:
Log a warning to debug log if unknown settings are encountered. This should probably only ever happen when the software is upgraded.
Something similar is already done for the command line and config file. See:
* test: Add test for unknown args #16234 (commit fa7dd88b71a1c6641bd450fae29a4a31849b1afd)
ACKs for top commit:
ryanofsky:
Code review ACK fa48405ef84985e5a9d38ec38e90d16596ea45b5. Looks good and I could see this being helpful for debugging. Thanks for taking suggestions
Tree-SHA512: cec7d88adf84fa0a842f56b26245157736eb50df433db951e622ea07fd145b899822b24cdab1d8b36c066415ce4f0ef09b493fa8a8d691532822a59c573aafa7
* fix(rpc): `masternode outputs` should produce an array
or it would ignore all but 1 outputs produced by the same tx
* rpc: improve `masternode outputs` help
* tests: check that `masternode outputs` show all outputs produced in the same tx
eca56f89293b74f11ca631ff2a0793e970e65841 test: replace 'regtest' leftovers by self.chain (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #16681 (fixes#18068), replacing all remaining hardcoded `"regtest"` strings in functional tests by `self.chain`.
Top commit has no ACKs.
Tree-SHA512: 96524649b33164938e5a95215991103ed7855ebab55ef788d4816b3fa5cbc03d8f3b0d39f2247a87522f289fd7f4daf25e059900b8462b5127eb154bbee89054
* coinjoin: make CCoinJoinServer managed pointer, assign CConnman during init
* coinjoin: make CCoinJoinClientQueueManager managed pointer, assign CConnman during init
* sporks: move spork validation logic downwards after CConnman initialization
* sporks: make CSporkManager a pointer, reduce global invocations
* governance: make CGovernanceManager a pointer, reduce global invocations
* llmq: migrate LLMQ subsystem raw pointers to managed pointers
* masternode: make activeMasternodeManager a managed pointer
* masternode: make masternodeSync a managed pointer, assign CConnman during init
* refactor: make instantsend helper functions class members
* fix: send empty CDeterministicMNList if pointer isn't initialized yet
* fix: refactor governance object retrieval logic across node and ui
Update src/interfaces/node.cpp
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1c23ea5fe67b88fd72a1ff640dd1bbb21a34fbf4 test: fix bitcoind already running warnings on macOS (fanquake)
Pull request description:
On macOS, `pidof` installed via brew returns b'' rather than None.
Account for this, to remove spurious warnings from the test_runner.
ACKs for top commit:
laanwj:
ACK 1c23ea5fe67b88fd72a1ff640dd1bbb21a34fbf4
Tree-SHA512: 640f4323d4105eac5c7abb52daf80486d5d3b4a074720490ceeb97c3dd8d73a3de9a988d2550f1e2076c620bb10d452b2959d8b723d2ee64f499878909824e31
fac942ca57dce6cfa5655a3ac8664d6a051bc01f test: Remove fragile assert_memory_usage_stable (MarcoFalke)
Pull request description:
This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.
Example failures:
* https://travis-ci.org/bitcoin/bitcoin/jobs/611497963#L14517
* https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/611029104#L3876
ACKs for top commit:
jamesob:
ACK fac942ca57
Tree-SHA512: 3577e7ce5891d221cb798454589ba796ed0c06621a26351bb919c23bc6bb46aafcd0b11cb02bbfde64b74d67cb2950da44959a7ecdc436491a34e8b045c1ccf4
49997813a4db388b2810e5e27ef771e8aa6a1f03 test: check custom ancestor limit in mempool_packages.py (Sebastian Falbesoner)
Pull request description:
The functional test `mempool_packages.py` starts one node with default ancestor/descendant limit settings and one with a custom, reduced ancestor limit (currently `-limitancestorcount=5`). The effect of the latter had not been tested yet though. This is approached in this PR by checking on the expected mempool contents of node1 after the node0 ancestor tests are done, via the following three conditions:
- the # of txs in the node1 mempool is equal to the the limit
- all txs in node1 mempool are a subset of txs in node0 mempool
- the node1 mempool txs match the start of the constructed tx-chain
Note that this still doesn't *fully* check the expected mempool of node1 (e.g. that it isn't influenced by `prioritisetransaction` RPC on node0), hence I add another TODO. In the future it would make sense to also set a custom descendant limit when the second TODO about checking node1's mempool is approached: 89e93135ae/test/functional/mempool_packages.py (L228)
ACKs for top commit:
MarcoFalke:
ACK 49997813a4db388b2810e5e27ef771e8aa6a1f03 👲
Tree-SHA512: d3a1d19fb49731238ad08ee7c02e2fa81a227e3b4ef3340d68598de42ddb62be9161134f6b8e08fa76b8c9faa02fecfa01111159642e20e9f358292a757b7608
af7bae734089f6af0029b0887932ccd9a469e12e [tests] Don't stop-start unnecessarily in rpc_fundrawtransaction.py (John Newbery)
9a8505299ba392acbab4647963113b0c29495f1d [tests] Use -whitelist in rpc_fundrawtransaction.py (John Newbery)
646b593bbd0db113c6e45ab92177b8f5251e8710 [tests] Speed up rpc_fundrawtransaction.py (John Newbery)
Pull request description:
Speed up rpc_fundrawtransaction.py
Most of the time in rpc_fundrawtransaction.py is spent waiting for
unconfirmed transactions to propagate. Net processing adds a poisson
random delay to the time it will INV transactions with a mean interval
of 5 seconds. Calls like the following:
```
self.nodes[2].sendrawtransaction(signedTx['hex'])
self.sync_all()
self.nodes[1].generate(1)
````
will therefore introduce a delay waiting for the mempools to sync.
Instead just generate the block on the node that sent the transaction:
```
self.nodes[2].sendrawtransaction(signedTx['hex'])
self.nodes[2].generate(1)
```
rpc_fundrawtransaction.py is not intended to be a test for transaction
relay, so it's ok to do this.
ACKs for top commit:
MarcoFalke:
ACK af7bae734089f6af0029b0887932ccd9a469e12e 🛴
Tree-SHA512: db3407d871bfdc99a02e7304b07239dd3585ac47f27f020f1a70608b7f6386b134343c01f3e4d1c246ce734676755897671999695068d6388602fb042d178780
fa5facd3e72b6d61374b0b93b722b55e2b090020 rpc: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, the rpc threads are `std::thread`, which does not have an `std:🧵:interrupt` member function to request interruption: https://dev.visucore.com/bitcoin/doxygen/httpserver_8cpp.html#ae1a63374e18b9abd348eb74e4243ea34
Thus, the interruption points can be removed.
ACKs for top commit:
laanwj:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020, this does nothing.
practicalswift:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020
jamesob:
ACK fa5facd3e7
Tree-SHA512: 4e29a44df1f2702cbd1ffdffa559440a8bb800baab64b4116e2c3d27cd64d8d1e8aafe1dc21b1a4e3988470d03be19cae294bd5669f7abf6d487685dc8fd8d7e
436ad436434b94982bcb7dc1d13a21949263ef73 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)
Pull request description:
Closes#8752 by bringing back abandoned #10470.
This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.
For more context, #8757 was closed in favor of #10470.
ACKs for top commit:
instagibbs:
utACK 436ad43643
kallewoof:
utACK 436ad436434b94982bcb7dc1d13a21949263ef73
jonatack:
I'm not qualifed to give an ACK here but 436ad436434b94982bcb7dc1d13a21949263ef73 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:
Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
72eaab073bc747425fe551777154b13a6c4c37c9 tests: functional watch-only wallet tests (William Casarin)
72ffbdc5799c1707ecad674d701b43fb80b031d0 doc: add release note for include_watchonly default changes (William Casarin)
003a3c73c0450aa18ac2ab2ca47def2b8c53a7df rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6c0b8e8144d3deec58ec2e3449ba081151 rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)
Pull request description:
Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.
Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.
ACKs for top commit:
achow101:
Code review ACK 72eaab073bc747425fe551777154b13a6c4c37c9
Sjors:
ACK 72eaab073bc747425fe551777154b13a6c4c37c9
promag:
ACK 72eaab073bc747425fe551777154b13a6c4c37c9, code review only, didn't look closely to the test.
kallewoof:
ACK 72eaab073bc747425fe551777154b13a6c4c37c9
fanquake:
ACK 72eaab073bc747425fe551777154b13a6c4c37c9 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.
Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7
* Move wallet enums to walletutil.h
* MOVEONLY: Move key handling code out of wallet to keyman file
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
* Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
* Renamed classes in scriptpubkeyman
* Fixes for conflicts, compilation and linkage errors due to previous commits
* Reordered methods in scriptpubkeyman to make further backports easier
* Reordered methods in scriptpubkeyman to make further backports easier (part II)
* Remove HDChain copy from SigningProvider class
* fixes/suggestions
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* refactor(llmq): substitute memberless class llmq::CLLMQUtils with namespace llmq::utils
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* chore: mark functions internal to `llmq::utils` as `static`
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* fix(llmq): Ensure connections between quorums
Every masternode will now "watch" a single node from _every other_ quorum in addition to intra-quorum connections. This should make propagation of recsigs produced by one quorum to other quorums much more reliable.
* fix: Do this only for masternodes which participate in IS quorums
* refactor: rename `CQuorumManager::EnsureQuorumConnections` to better match the actual behaviour
(and avoid confusion with `CLLMQUtils::EnsureQuorumConnections`)
* refactor: move IS quorums watch logic into `CQuorumManager::CheckQuorumConnections`
avoid calling slow `ScanQuorums` (no caching atm) inside the loop
* tests: check that inter-quorum connections are added
* use `ranges::any_of`
* fix(llmq): mark mns "bad" based on the failed connect attempts count
Avoid using "last success time" as a proxy
* fix(tests): tweak feature_llmq_simplepose.py
* Add HaveKey and HaveCScript to SigningProvider
* Remove CKeyStore and squash into CBasicKeyStore
* Move HaveKey static function from keystore to rpcwallet where it is used
* scripted-diff: rename CBasicKeyStore to FillableSigningProvider
-BEGIN VERIFY SCRIPT-
git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g'
-END VERIFY SCRIPT-
* Move KeyOriginInfo to its own header file
* Move various SigningProviders to signingprovider.{cpp,h}
Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.
Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'
* Remove CCryptoKeyStore and move all of it's functionality into CWallet
Instead of having a separate CCryptoKeyStore that handles the encryption
stuff, just roll it all into CWallet.
* Fixed cases of mess CWallet functions with CCryptoKeyStore and conflicts
* Move WatchOnly stuff from SigningProvider to CWallet
* Fixes for lint cirtular dependencies to calm linter
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
0ea5d70b4756f376342417e0019490233cb4a918 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8eb7bb97531a967f5d1dcc7b8865d1e Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)
Pull request description:
Related to: #18428
When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.
ACKs for top commit:
MarcoFalke:
ACK 0ea5d70b4756f376342417e0019490233cb4a918
Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
26fe9b990995f9cb5eee21d40b4daaad19f7181f Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2e2fb31bc362c848bd2456d576e408b Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33e824f5abb6a68452c683d6ce8b3e4a Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f887348a08c620732125ad4430e1a133d434 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b990995f9cb5eee21d40b4daaad19f7181f
laanwj:
utACK 26fe9b990995f9cb5eee21d40b4daaad19f7181f (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
* Remove unused variable
* [refactor] Move tx relay state to separate structure
* [refactor] Change tx_relay structure to be unique_ptr
* Check that tx_relay is initialized before access
* Add comment explaining intended use of m_tx_relay
* Add 2 outbound block-relay-only connections
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.
(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
* Don't relay addr messages to block-relay-only peers
We don't want relay of addr messages to leak information about
these network links.
* doc: improve comments relating to block-relay-only peers
* Disconnect peers violating blocks-only mode
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
* net_processing. Removed comment + fixed formatting
* Refactoring net_processing, removed duplicated code
* Refactor some bool in a many-arguments function to enum
It's made to avoid possible typos with arguments, because some of them have default values and it's very high probability to make a mistake here.
* Added UI debug option for Outbound
* Fixed data race related to `setInventoryTxToSend`, introduced in `[refactor] Move tx relay state to separate structure`
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
0e7c90eb37a687158c261ddd1ff9f1028a1e7012 test: speed up wallet_avoidreuse.py (Jon Atack)
6d50b2606ea9249627556051637080c3587b1b04 test: add logging to wallet_avoidreuse.py (Jon Atack)
Pull request description:
Inspired by PRs #17340 and #15881.
- add logging
- pass -whitelist in `set_test_params` to speed up transaction relay
`wallet_avoidreuse.py` is not intended to test P2P transaction relay/timing, so it should be fine to do this here. This reduces test run time variability and speeds up the test by 2-3 times on average.
Test run times in seconds:
- before: 20, 24, 22, 17, 27, 40, 30
- after: 10, 10, 8, 9, 10, 7, 8
ACKs for top commit:
MarcoFalke:
ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012 🐊
fanquake:
ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012
Tree-SHA512: 6d954a0aaf402c9594201626b59d29263479059e68fa5155bb44ed973cd0c3347729dd78b78b4d5a2275e45da365dc1afb4cc7e3293dea33fcc2e3e83a39faf5
5ebc6b0eb267e0552c66fffc5e5afe7df8becf80 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8f92d44d893cf9f22d15acdeca40b1a doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da52099e4a6a401acd7aa32b32832423 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c0468f9ba92bc789a698281b1c81284 test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf4015de87741ff549db35e5064f4e16 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d5883309cb0dd14b826bc45c5e776fb wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a46d7c38943ee0304343ab7465305bd wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fad917b169f5e3b8baaf4301dcf00a7b wallet: avoid reuse flags (Karl-Johan Alm)
58928098c299efdc7c5ddf2dc20716ca5272f21b wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9a3efa2fa16d780885048a06566d262 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb267e0552c66fffc5e5afe7df8becf80
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb267e0552c66fffc5e5afe7df8becf80 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
ea3c7e585c382998212fd7f41114462a8168a734 test: Remove libssl-dev packages from CI scripts (Wladimir J. van der Laan)
7ea55264b9d60325bc7a5c15d78e9063de145970 test: remove lsan suppression for libcrypto (Wladimir J. van der Laan)
2d7066527a456f8e1f4f603fe104b0bd9d864559 build: remove libcrypto as internal dependency in libbitcoinconsensus.pc (Wladimir J. van der Laan)
278751ea11f2cfe68b0c98f504f65586720cb5a4 doc: Remove ssl as a required dependency from build-unix (Wladimir J. van der Laan)
Pull request description:
Some doc and build cleanups following #17265.
I intentionally left the libssl-dev install in `gitian-win-signer.yml`, as it's necessary for the ossl signer.
ACKs for top commit:
MarcoFalke:
ACK ea3c7e585c382998212fd7f41114462a8168a734 🗯
jamesob:
ACK ea3c7e585c
practicalswift:
ACK ea3c7e585c382998212fd7f41114462a8168a734 - nice!
fanquake:
ACK ea3c7e585c382998212fd7f41114462a8168a734 - thanks.
Tree-SHA512: 67ea35bdd6d6e512d69e6734713534c88cae033a2ed695677ea15c3e3d5ff570374e342775c88e60877fa43a19047853e7b2a433e2c9a4349a5c423726a7457e
aa410c2b17 rpc: Validate maxfeerate with AmountFromValue (João Barbosa)
Pull request description:
With this change `maxfeerate` can also be set as a string, accordingly to the help test:
```
maxfeerate (numeric or string,
```
Beside, there are no tests for the removed errors.
ACKs for commit aa410c:
meshcollider:
utACK aa410c2b17
MarcoFalke:
utACK aa410c2b17 Good catch
Tree-SHA512: f3bfea91dc7daa943729e270585dbf333055aeda805fbd01eaab20a7e0e6147382647c11525334382d198df0d3d45da6102b541efda5a1361f96271c98d5d89d
90df92206cfce4e61eff9d584112643512f6b91c test: Change filemode of rpc_whitelist.py (Emil Engler)
Pull request description:
All python tests have the file mode `755`.
Probably due to a mistake `rpc_whitelist.py` is the only test with the permission `644`.
This PR makes it coherent with the other tests and updates it to `755` as well.
ACKs for top commit:
practicalswift:
ACK 90df92206cfce4e61eff9d584112643512f6b91c -- all tests should be executable
Tree-SHA512: b9e69cb5184a3bbee4c7b14ac35985145a9fd3403d0e449d79f15c18e9660cafec495d639f5f730e0c69dde5f4a3d7590b4e42d385e794cd02add1f4e3b785e7
2081442c421cc4376e5d7839f68fbe7630e89103 test: Add test for rpc_whitelist (Emil Engler)
7414d3820c833566b4f48c6c120a18bf53978c55 Add RPC Whitelist Feature from #12248 (Jeremy Rubin)
Pull request description:
Summary
====
This patch adds the RPC whitelisting feature requested in #12248. RPC Whitelists help enforce application policies for services being built on top of Bitcoin Core (e.g., your Lightning Node maybe shouldn't be adding new peers). The aim of this PR is not to make it advisable to connect your Bitcoin node to arbitrary services, but to reduce risk and prevent unintended access.
Using RPC Whitelists
====
The way it works is you specify (in your bitcoin.conf) configurations such as
```
rpcauth=user1:4cc74397d6e9972e5ee7671fd241$11849357f26a5be7809c68a032bc2b16ab5dcf6348ef3ed1cf30dae47b8bcc71
rpcauth=user2:181b4a25317bff60f3749adee7d6bca0$d9c331474f1322975fa170a2ffbcb176ba11644211746b27c1d317f265dd4ada
rpcauth=user3:a6c8a511b53b1edcf69c36984985e$13cfba0e626db19061c9d61fa58e712d0319c11db97ad845fa84517f454f6675
rpcwhitelist=user1:getnetworkinfo
rpcwhitelist=user2:getnetworkinfo,getwalletinfo, getbestblockhash
rpcwhitelistdefault=0
```
Now user1 can only call getnetworkinfo, user2 can only call getnetworkinfo or getwalletinfo, while user3 can still call all RPCs.
If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.
Review Request
=====
In addition to normal review, would love specific review from someone working on LN (e.g., @ roasbeef) and someone working on an infrastructure team at an exchange (e.g., @ jimpo) to check that this works well with their system.
Notes
=====
The rpc list is spelling sensitive -- whitespace is stripped though. Spelling errors fail towards the RPC call being blocked, which is safer.
It was unclear to me if HTTPReq_JSONRPC is the best function to patch this functionality into, or if it would be better to place it in exec or somewhere else.
It was also unclear to me if it would be preferred to cache the whitelists on startup or parse them on every RPC as is done with multiUserAuthorized. I opted for the cached approach as I thought it was a bit cleaner.
Future Work
=====
In a future PR, I would like to add an inheritance scheme. This seemed more controversial so I didn't want to include that here. Inheritance semantics are tricky, but it would also make these whitelists easier to read.
It also might be good to add a `getrpcwhitelist` command to facilitate permission discovery.
Tests
=====
Thanks to @ emilengler for adding tests for this feature. The tests cover all cases except for where `rpcwhitelistdefault=1` is used, given difficulties around testing with the current test framework.
ACKs for top commit:
laanwj:
ACK 2081442c421cc4376e5d7839f68fbe7630e89103
Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf
b6f9e3576a1ea18572e4803aeb3f39330f0cb759 test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/17675#issuecomment-563188648.
ACKs for top commit:
practicalswift:
ACK b6f9e3576a1ea18572e4803aeb3f39330f0cb759 assuming Travis is happy too -- diff looks correct :)
MarcoFalke:
> ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
1583498fb6781c01ca2f33c09319ed793964c574 Send and require SENDADDRV2 before VERACK (Pieter Wuille)
c5a89196602e43ebb1cdc9cd4f08d153419c13e1 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille)
Pull request description:
BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some implementations reject messages they don't know. As a courtesy, don't send it to nodes with a version before 70016, as no software is known to support BIP155 that doesn't announce at least that protocol version number.
Also move the sending of sendaddrv2 earlier (before sending verack), as proposed in https://github.com/bitcoin/bips/pull/1043. This has the side effect that local address broadcast of torv3 will work (as it'll only trigger after we know whether or not the peer supports addrv2).
ACKs for top commit:
MarcoFalke:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jnewbery:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jonatack:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
vasild:
ACK 1583498
Tree-SHA512: 3bd5833fa8c8567b6dedd99e4a9b6bb71c127aa66d5284b217503c86d597dc59aa7382c41f3a4bf561bb658b89db81d1a7703a700eef4ffc17cb916660e23a82
fa949b3c1325693ea7ecc5556b2de50d2a6c9ead test: Suppress epoll_ctl data race (MarcoFalke)
Pull request description:
Happens intermittently: https://cirrus-ci.com/task/5462892373868544?command=ci#L5385
ACKs for top commit:
hebasto:
ACK fa949b3c1325693ea7ecc5556b2de50d2a6c9ead, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d5aa559fc105053da594531722f2a03d898eadeb4413c3a728fc5116cc4d1a2c16c49649a24c75ea810e4ec6bb9728b0bcd2ea991886bb9d206170218eddf6d2
a51d0ad2de89b9757d158df95ddeba2bfcb23935 rpc: Improve addnode remove command error message (Fabian Jahr)
Pull request description:
The `addnode` RPC with the `remove` command parameter is used to remove a node from the "added nodes". It did not have test coverage and in case of failure to remove the node it responded with the confusing message "Error: Node has not been added.".
This PR adds test coverage and introduces a new error code as well as changes the error message to something that makes sense.
ACKs for top commit:
laanwj:
Code review ACK a51d0ad2de89b9757d158df95ddeba2bfcb23935
theStack:
Tested ACK https://github.com/bitcoin/bitcoin/commit/a51d0ad2de
Tree-SHA512: 033ef5de0d4d49d58ef4df3759b838c9d19ee9dfb0aff9f814a3a63d124ca231a442c930efa7d343fe1f65727c4b59fc23dd5e26fe6ea69f9e84fda48b5c5cc2
ff22751417c6fbbd22f4eefd0e23431a83335c13 test: rm ascii art in rpc_fundrawtransaction (Jon Atack)
94fcc08541cf58bee864ab7c28a6c77e42472f17 test: add rpc_fundrawtransaction logging (Jon Atack)
Pull request description:
`test/functional/rpc_fundrawtransaction.py` is fairly slow to run and has no logging, so it can appear to be stalled.
This commit adds info logging at each test to provide feedback on the test run.
ACKs for top commit:
instagibbs:
utACK ff22751417
jnewbery:
tACK ff22751417c6fbbd22f4eefd0e23431a83335c13
Tree-SHA512: f4fabad8ef51c29981351bb4e66fb0c0e0517418a4a15892ef804df11d16b2d2ae1a1abc958d2b121819850278de90a2003b0edb8d7098d00360b89fa76e9062
f3f6dde56e Test coinbase category in wallet rpcs (andrewtoth)
e982f0b682 Add all category options to wallet rpc help (andrewtoth)
Pull request description:
The current helptext for `listtransactions`, `listsinceblock` and `gettransaction` only list two of the five possible options for `category`. This incorrectly implies that these are the only two options, and can cause problems if the other three options aren't accounted for. Also, some of the documentation is incorrect when specifying which options are returned for which categories.
This PR updates the helptext for these RPCs and adds a functional regression test for the cases when the other three categories are returned.
Tree-SHA512: 67dd7ff6269a3b0f17f5d1a61b0ae1fb1f3778f05e1c440bfbb9b3a005c9c6d740abcace20f3d597cf2bd6779c494448690f13fab0bd2340f206213bc7890b51
eaf4070e3a Add suppression for InterruptRPC (fRPCRunning) data race (practicalswift)
5e5138a721 travis: Use trap and set -e errtrace (Chun Kuan Lee)
069752b726 build: Enable functional tests in the ThreadSanitizer (TSan) build job (practicalswift)
Pull request description:
Enable functional tests in the ThreadSanitizer (TSan) build job.
This is a follow-up to @MarcoFalke's #14764 which added TSan but for unit tests only.
Tree-SHA512: dcc24d311fa124772c3036b16c2bf94732ece36c3e22b4bb8fe941772e52157ab2b1a90b1880b81079c2eef2d344ca7e1da58324b75dbf82d16204d591ad49fb
0dcac51049 wallet_keypool_topup.py: Test for all keypool address types (Gregory Sanders)
Pull request description:
To protect against regressions if key scanning is changed.
Tree-SHA512: d1c4bb033bafd97203a3f68fb262a501442be947907d67902f0391fbdec39c095196403c7675e602806cc68d7e2d1f552ab339a58346162379978d06dad1c4bb
* Fix build of qtbase in contrib for Gcc 11.x
It adds a patch with missing include <limits> in qtbase/src/tools/moc/generator.cpp
* Merge bitcoin/bitcoin#23716: test: replace hashlib.ripemd160 with an own implementation
5b559dc7ecf37ab1604b75ec8ffe8436377a5fb1 Swap out hashlib.ripemd160 for own implementation (Pieter Wuille)
ad3e9e1f214d739e098c6ebbd300da5df1026a44 Add pure Python RIPEMD-160 (Pieter Wuille)
Pull request description:
Closes#23710.
ACKs for top commit:
jamesob:
ACK 5b559dc7ec, pending CI
Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
* Updates doc for Unix build: added missing dependency bison
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa815255c70d32809aac640db4a8762c7d71e8db test: Add missing sync_all to wallet_balance test (MarcoFalke)
Pull request description:
A `syncwithvalidationinterfacequeue` should be sufficient.
Fixes #16020
ACKs for top commit:
promag:
ACK fa81525. This can be tested by adding sleep in `CWallet::BlockConnected` just before `LOCK(cs_wallet)` - master will always fail while this PR will succeed.
Tree-SHA512: 07e067c698627f90f0b9848f921b7067adc70c27105db3258e056384197e50dbee055c87839d238cc11bde11179d3f5879b39e1c8e15465f8f07558c694b677d
89339d14607434b33cfa343dc75877b62b1dfe0e tests: Add test for loadblock option (Fabian Jahr)
Pull request description:
Fixes#17019
Was initially part of #17044 but as the test got larger it made sense to split it into its own commit as suggested in #17019 .
This is testing the `-loadblock` option by using the scripts in `contrib/linearize` to generate a `bootstrap.dat` file and starting a disconnected node with it. So it is also testing the linearize scripts which were untested before and needed to be made available for the CI environment, hence they are added to `DIST_CONTRIB` in `Makefile.am`.
ACKs for top commit:
laanwj:
ACK 89339d14607434b33cfa343dc75877b62b1dfe0e
Tree-SHA512: aede0cd6e8b21194973f3633bc07fa2672d66a6f85dfe6a57cee2bb269a65d19ea49d5f9ed7914a173b3847c76e70257aa865f44bde170c1999d9655b4862d1c
0616138a0797cf68ad869906c36cf0767e20b313 tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. (practicalswift)
Pull request description:
Remove no longer needed UBSan suppressions (issues fixed). Add documentation.
This PR is the CI-only subset of #17208 (which touches code).
From a fuzzing perspective it would be really nice to be able to run UBSan with as few suppressions as possible :)
Top commit has no ACKs.
Tree-SHA512: a926ab3e80e12a805af110fbff470cdc61ef4db536919a5b8896ea8b70f761114a52d9b1c0f48b11c1d48338351bf2e003e01ce60c613612f26ba298dcc29cd9
fa677d1801fb9153a95a1fc9855fd5f21fc440c0 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke)
fadccb263baf6b8694f750623add42f966e423a3 doc: Document that GNU tools are required for linters (MarcoFalke)
4444704ca9f66cdc24ab2d444941354db1dfed06 ci: Cleanup macOS runs (MarcoFalke)
Pull request description:
* Remove a commented out cleanup task in `before_cache`
* Remove the linter run on macOS, and document that GNU tools are required to run the linters
ACKs for top commit:
Sjors:
Code review ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0
laanwj:
ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0
ryanofsky:
Code review ACK fa677d1801fb9153a95a1fc9855fd5f21fc440c0 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting
Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
9576614d2d91ca946d164dfffca441f8bcbd6e2c doc: Describe log files + consistent paths in test READMEs (Martin Erlandsson)
Pull request description:
picks up #15830
I saw this was almost ready to merge but the test logging part was not 100% correct. I reworked that part, the rest is the same.
ACKs for top commit:
GChuf:
ACK 9576614d2d91ca946d164dfffca441f8bcbd6e2c
Tree-SHA512: 3de7f1b0a1b0419df6e7b55964d00e715b6cb7874b1849ad6f120597610d7df4182c4b61b9c9691ce04f4e392ed3caead4c623374be2066ac31319e702d45d09
32d665c2657793c8b2cc7248d26d80a940acfe20 test: fix "tx-size-small" errors after default address change (Sebastian Falbesoner)
Pull request description:
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see `src/validation.cpp:MemPoolAccept::PreChecks(...)`),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
```21 <21-byte-long string of 'a' or 1s>```
ACKs for top commit:
instagibbs:
reACK 32d665c265 just s/Bytes/bytes/
MarcoFalke:
ACK 32d665c2657793c8b2cc7248d26d80a940acfe20
Tree-SHA512: 80e0386ff3c3f462901ba5c1e5ef2cbf095d9c0a40c8c3cfeacd4a3ab676afe744aa95b9eed77b4b3eec88bed930b33aa718117ed0977f6374e858a2f3bd5c57
d20d756752 rpc: faster getblockstats using BlockUndo data (Felix Weis)
Pull request description:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.
```
# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200 0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats 0.21s user 0.19s system 17% cpu 2.302 total
# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200 0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats 0.24s user 0.22s system 0% cpu 3:19.42 total
```
ACKs for commit d20d75:
MarcoFalke:
re-utACK d20d7567528e216badb8475df298bb3cec008985
Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
3fd7e76f6d [tests] Move deterministic address import to setup_nodes (John Newbery)
Pull request description:
This requires a small changes to a few tests, but means that
deterministic addresses will always be imported (unless setup_nodes
behaviour is explicitly overridden).
Tidies up the way we import deterministic addresses, requested in review comment here: https://github.com/bitcoin/bitcoin/pull/14468#discussion_r225594586.
Tree-SHA512: 2b32edf500e286c463398487ab1153116a1dc90f64a53614716373311abdc83d8a251fdd8f42d1146b56e308664deaf62952113f66e98bc37f23968096d1a961
07cae5287c [wallet] remove unused GetScriptForMining (Sjors Provoost)
8bb3e4c487 [rpc] remove deprecated generate method (Sjors Provoost)
Pull request description:
As announced in v0.18, the wallet generate rpc method is deprecated and will be fully removed in v0.19.
Clients should transition to using the node rpc method `generatetoaddress`.
Tree-SHA512: 9e5e913b59f3e18440b2b7b356124c7b87ad19f81a1ab6ada06a6c396b84e734895465f569296f1ba8c12abf74863bab5fd77765c9e806c239713aa83a59485f
8f7b93047581c67f2133cdb8c7845471de66c30f Drop the leading 0 from the version number (Andrew Chow)
Pull request description:
Removes the leading 0 from the version number. The minor version, which we had been using as the major version, is now the major version. The revision, which we had been using as the minor version, is now the minor version. The revision number is dropped. The build number is promoted to being part of the version number. This also avoids issues where it was accidentally not included in the version number.
The CLIENT_VERSION remains the same format as previous as previously, as the Major version was 0 so it never actually got included in it.
The user agent string formatter is updated to follow this new versioning.
***
Honestly I'm just tired of all of the people asking for "1.0" that maybe this'll shut them up. Skip the whole 1.0 thing and go straight to version 22.0!
Also, this means that the terminology we commonly use lines up with how the variables are named. So major versions are actually bumping the major version number, etc.
ACKs for top commit:
jnewbery:
Code review ACK 8f7b930475
MarcoFalke:
review ACK 8f7b93047581c67f2133cdb8c7845471de66c30f 🎻
Tree-SHA512: b5c3fae14d4c0a9c0ab3b1db7c949ecc0ac3537646306b13d98dd0efc17c489cdd16d43f0a24aaa28e9c4a92ea360500e05480a335b03f9fb308010cdd93a436
cf9ed307e6 qa: blocktools enforce named args for amount (MarcoFalke)
Pull request description:
Since #13669 changed some signatures, I think it might be worthwhile to enforce named args for primitive types such as amounts.
Tree-SHA512: 2733e7b6a20590b54bd54e81a09e3f5e2fadf4390bed594916b70729bcf485b048266012c1203369e0968032a2c6a2719107ac17ee925d8939af3df916eab1a6
Issues with current implementation: params list is not mentioning `baseBlockHashes`, `baseBlockHashesNb` looks excessive, no default values, handling of baseBlockHash-es is off by 1 (`3 + i` should be `4 + i`).
before:
```
> help quorum rotationinfo
quorum rotationinfo "blockRequestHash" baseBlockHashesNb extraShare
Get quorum rotation information
Arguments:
1. blockRequestHash (string, required) The blockHash of the request.
2. baseBlockHashesNb (numeric, required) Number of baseBlockHashes
3. extraShare (boolean, required) Extra share
```
after:
```
> help quorum rotationinfo
quorum rotationinfo "blockRequestHash" ( extraShare "baseBlockHash..." )
Get quorum rotation information
Arguments:
1. blockRequestHash (string, required) The blockHash of the request.
2. extraShare (boolean, optional, default=false) Extra share
3. baseBlockHash... (string, optional, default=) baseBlockHashes
```
* tests: move `move_to_next_cycle` to `DashTestFramework`
* tests: set correct defaults for `mine_cycle_quorum`
* tests: use correct quorum type in `create_islock`
* tests: fix `rpc_verifyislock.py`
* tests: fix `feature_llmq_is_cl_conflicts.py`
* tests: isolate zmq subscribers in `interface_zmq_dash.py`
this lets us call `test_*_publishers()` in any order and any number of times
* tests: check zmq for both deterministic and non-deterministic islocks
27fcb40fc0c9ba5608a982be7372924b1edefb17 doc: replace outdated OpenSSL comment in test README (fanquake)
Pull request description:
The OpenSSL dependency was removed in #15826.
ACKs for top commit:
laanwj:
ACK 27fcb40fc0c9ba5608a982be7372924b1edefb17
theStack:
ACK 27fcb40fc0
Tree-SHA512: eb7a3b18fefa91e6f27c50fa065d6cc330f7b633ae8ee51145cdeec4df51dea5155f0d1fa91e75f1202adef04e063f3eda12773cd00a093f29f5a5e83c4fda73
* tests: fix `wait_for_*` methods
should not override `all_ok` once it's `False`
* tests: force non-mns to switch to "fully synced" mnsync status in `setup_network`
like we do this for the controller node and all mns already
f1a0314c537791f202dfb7c1209f0e04ba7988c3 gui: change combiner for signals to optional_last_value (Cory Fields)
Pull request description:
[`optional_last_value`](https://www.boost.org/doc/libs/1_73_0/doc/html/boost/signals2/optional_last_value.html), which does not throw, has replaced `last_value` as
Boosts default combiner. Besides being better supported, it also doesn't
trigger gcc's `-Wmaybe-unitialized` warning, presumably because exceptions no
longer bubble-up out of signals:
```bash
In file included from ui_interface.cpp:9:
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
if(value) return value.get();
^
/bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
optional<T> value;
^~~~~
```
The change in default happened in [Boost 1.39.0](https://www.boost.org/users/history/version_1_39_0.html) (along with the introduction of the Signals2 library.
More information is also available here https://www.boost.org/doc/libs/1_73_0/doc/html/signals2/rationale.html#id-1.3.36.9.4:
> The default combiner for Boost.Signals2 has changed from the last_value combiner used by default in the original Boost.Signals library.
> This is because last_value requires that at least 1 slot be connected to the signal when it is invoked (except for the last_value<void> specialization).
> In a multi-threaded environment where signal invocations and slot connections and disconnections may be happening concurrently, it is difficult to fulfill this requirement. When using optional_last_value, there is no requirement for slots to be connected when a signal is invoked, since in that case the combiner may simply return an empty boost::optional.
ACKs for top commit:
laanwj:
ACK f1a0314c537791f202dfb7c1209f0e04ba7988c3
Tree-SHA512: 3600f85019a3591b141dc9207f8a7e66d16d9996cf97fdf08f5133a212d55c591955ab835ffbdca20b5d62711578bc305d5525c75546fa957f180192e2a80c1e
* Added GET_SNAPSHOT_INFO message handling
* Quorum members by rotation
* Quorum utils functions
* Handle GET_QUORUM_ROTATION_INFO with baseBlockHash from client
* Storing QuorumSnaphots in evoDB when requesting them
* Added DIP Enforcement param
* quorumIndex cache
* Quorum Rotation deployment control
* Usage of Bitsets for storing CQuorumSnapshots
* Correct handling of early quorum quarters
* More asserts
* Corrections
* Handling of quorumIndex
* Refactoring of truncate mechanism
* Various fixes
* Interface correction
* Added template type for indexed cache
* Added quorumIndex into commitmenHash
* Various changes
* Needs to update maqQuorumsCache along with indexedQuorumsCache
* Added CFinalCommitment version 2
* Renamed variables
* Fixes
* Refactoring & correct caching of quorumMembers by rotation
* Added assertions
* Refactoring
* Interface change
* Handling of previous DKG session failure
* Applied refactoring
* Build quarter members improvments
* Merge Quorum Rotation and Decreased fee into one deployment (DIP24)
* Added new LLMQ Type
* Added functional tests + refactoring
* Refactoring
* Spreaded Quorum creation and Quorum Index adaptation
* quorumIndex adaptations
* Added quorumIndex in CFinalCommitment
* Latest work
* Final refactoring
* Batch of refactoring
* Fixes for tests
* Fix for CFinalCommitment
* Fix for Quorums
* Fix
* Small changes
* Thread sync fic
* Safety changes
* Reuse mns when needed
* Refactoring
* More refactoring
* Fixes for rotationinfo handling
* Fix for rotation of members
* Correct order of MNs lists in Quorum Snapshots
* Adding extra logs
* Sync rotation quorums + qrinfo changes
* Fix + extra logs
* Removed redundant field
* Fix for null final commitment + refactoring
* Added timers in tests
* Fix for qrinfo message: quorumdiff and merkleRootQuorums
* Small changes for rotation test
* Remove reading from scanQuorumCache
* Added quorum list output
* Crash fix
* Experimental commit
* apply changes to specialtxman.cpp from specialtx.cpp
* all the changes
* substancially speed up feature_llmq_rotation.py
* reenable asserts, add check for reorgs
* Refactoring
* Added extra logs
* format
* trivial
* drop extra boost includes
* drop ContainsMN
* fix ScanQuorums
* check quorum hash and index in CFinalCommitment::Verify
* fix/tweak tests
* IsQuorumRotationEnabled should be aware of the context
* Calculating members based on earlier block.
* Fix for Quorum Members Cache
* Removed duplicate size of baseBlockHashes
* Adaptations of qrinfo to -8 mn lists
* Introduction of llmqTypeDIP24InstantSend
* Adaptation for llmqTypeDIP24InstantSend
* Adaptations for IS
* bump protocol version
* Added feature_llmq_is_migration test
* Various cleanups
* use unordered_lru_cache for quorumSnapshotCache
* trivial refactor ComputeQuorumMembersByQuarterRotation
* Reduced CFinalCommitment::quorumIndex from 32 to 16 bits
* Keep verified LLMQ relay connections
* Experimental Relay connection fix
* Fix for EnsureQuorumConnections rotation
* Using only valid Mns for checking
* Override of nPowTargetSpacing (devnet only)
* Show penalty score in masternode rpc
* fixups
* Rotation refactoring
* Update src/chainparams.cpp
* Replaced LogPrintf with LogPrint
* IS locking fix once DIP24 activation
* Various cleanup
* Updated MIN_MASTERNODE_PROTO_VERSION
* Introduce LLMQ_TEST_INSTANTSEND reg-test only quorum and actually test switching to dip0024 quorums
* Renamed field lastQuorumHashPerIndex
* Renamed to DIP0024
* chore: update nStartTime and nTimeout for mainnet / testnet for DEPLOYMENT_DIP0024
Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
460fa8e0d9c1d08b5b5bca3fa02ba2763337aaf1 test: remove `import socket` in test_ipv6_local (brunoerg)
Pull request description:
Since this module (`socket`) is imported at the top of file, there is no need to import it again within the function.
ACKs for top commit:
MarcoFalke:
cr ACK 460fa8e0d9c1d08b5b5bca3fa02ba2763337aaf1
Tree-SHA512: 031c17a776dedaa21b3ec6458ca822304e76a5a3f4494406e6b7b04f08cc2abefcfe742c462b60c9b3e2fee3cd110a69ed5ad413357886dc7b823abc916ea40e
d8dfc403f74858b79c87f650b5d609aa60a4dc3b script: redirecting stderr to stdout before pipelining into grep (anouar kappitou)
30df5c3dd4d39d9027b0341d01d3233400104893 script: preventing non-compatible sed binary. (anouar kappitou)
Pull request description:
This Pull request improve scripted diff by checking for non-compatible sed binary. Fixes#19815
ACKs for top commit:
shaavan:
reACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b
theStack:
Tested ACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b
Tree-SHA512: 4813abb02195e04b8953662e51cba4599789e6185a6d384f56255e2d37c2b16be46c7c8861e44ba1a13d42573a58eb708901472925cb308dba2f602020e9f0f6
140a49ce5e547a1b520a7cd063af8308184e7cbf test: check that pruneblockchain RPC fails for future block or timestamp (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `pruneblockchain` RPC for the case that a future block or timestamp is passed:
8c0bd871fc/src/rpc/blockchain.cpp (L1101)8c0bd871fc/src/rpc/blockchain.cpp (L1111)
Note that the test method `manual_test` gets called twice, once each with `use_timestamp` set to True/False, respectively. Depending on that, the helper function `height` either converts the passed block height to the timestamp of that block, or just returns it without modification.
The other tests for failures in this RPC are also changed to be more detailled ("Cannot prune blocks because node is not in prune mode", "Negative block height"), as I don't think there is any value in just checking a sub-string. If there is ever an error with the same sub-string is introduced, it's not clear which error is exactly checked with the test, so it makes sense to be as specific as possible.
ACKs for top commit:
brunoerg:
tACK 140a49ce5e547a1b520a7cd063af8308184e7cbf
Tree-SHA512: bee3cee9f35c2a63a1839d7ec1f83e354d9d3c0c2ca32d300dca2de8b755d555f769ba2b80ac37d31df6ee7e2b8eaefb8134c4727a7144e47c0f5e34f2cc5822
4105a543817c47bda21739d5ad3269677f84e861 lint: remove boost::bind linter (fanquake)
Pull request description:
I don't think we need to maintain a linter for reintroducing boost::bind at this point.
ACKs for top commit:
hebasto:
ACK 4105a543817c47bda21739d5ad3269677f84e861, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 86bda91ee7ed11f0aa7ac95e9e7b62dbba626dcea75444d2851a3d40e794ab16bef09a1f0c956a716d43602b23c1cf67e1ff3a51184ea1ee7d686fbb76316cb0
fa6c62f34b50818102ad58f2ffd152189f54d973 test: Replace log with assert_equal in wallet_abandonconflict (MarcoFalke)
Pull request description:
This will make the test fail as soon as the bug is fixed, forcing it to
be updated. Otherwise, the bug might be fixed (or made worse)
accidentally, leaving the test in an inconsistent state.
ACKs for top commit:
theStack:
Concept and code-review ACK fa6c62f34b50818102ad58f2ffd152189f54d973
brunoerg:
tACK fa6c62f34b50818102ad58f2ffd152189f54d973
Tree-SHA512: 416f72380164bf3f93332a5cfa81a8e61d8ada3714ef6815889ed3cf2d16c09411760dee4acf19629227e565b765b3dea491a0b23efd38eb370254cfadf7c441
bda620aecd690004c52e550ad7de187ce0eb655d test: check abandoned tx in listsinceblock (brunoerg)
Pull request description:
This PR tests if the abandoned transaction is correct in listsinceblock return (wallet_abandonconflict.py).
ACKs for top commit:
jonatack:
ACK bda620aecd690004c52e550ad7de187ce0eb655d
theStack:
ACK bda620aecd690004c52e550ad7de187ce0eb655d
stratospher:
Tested ACK bda620a. This PR verifies whether the transaction txAB1 has been abandoned in listsinceblock and is a nice addition to the test!
Tree-SHA512: e4dce344cf621de7a8b5bd8660d252419772a293080fc881f6f448b6df85c6b1c8f0df619e855a40b6393f53c836f0d7fadbd3916c20cccd3a95830b8b502991
502f50da12694dd0193744427f0f89238e612f54 Test transactions conflicted by double spend in listtransactions (Jon Atack)
Pull request description:
Test the properties of transactions conflicted by a double spend as returned by RPC listtransactions in the abandoned, confirmations, trusted and walletconflicts fields. These fields are also returned by RPCs listsinceblock and gettransactions.
ACKs for top commit:
brunoerg:
tACK 502f50da12694dd0193744427f0f89238e612f54
rajarshimaitra:
Concept + tACK 502f50da12
Tree-SHA512: 28968f4a5f1960ea45b2e6f5b20fe25c1b51f66944062dcddea52ea970ad21c74d583793d091b84e8a5e506d6aecc1f0435c5b918213975b22c38e02bba19aa1
54b5eb2b149a1f2a4a1dbdb9e0648c5a390d8e22 tests: Add std::locale::global to list of locale dependent functions in lint-locale-dependence.sh (practicalswift)
Pull request description:
Add `std::locale::global` to list of locale dependent functions in `lint-locale-dependence.sh`.
We currently flag `setlocale(...)` as locale dependent, but prior to this commit we didn't flag
`std::locale::global(...)` as such.
In addition to setting the global C++ locale `std::locale::global(...)` also does the equivalent of `std::setlocale(LC_ALL, ...);`.
Thus the functionality of `std::locale::global(...)` is a superset of `setlocale(...)` :)
ACKs for top commit:
MarcoFalke:
ACK 54b5eb2b149a1f2a4a1dbdb9e0648c5a390d8e22, fine with me
Tree-SHA512: bcf2f1c765add6ed09c3debca968b75eeea81602503f109c0f76ec98635911d453f4834a39e741703c3d470f123178e8952191a9b1a3429394b99c07765dcf1f
e61de6306fd89fe9aae90253062e7b1b20343f8a Change ismine to take a CWallet instead of CKeyStore (Andrew Chow)
7c611e20007bf5face34d33dffa26c8db67e29ec Move ismine to wallet module (Andrew Chow)
Pull request description:
`IsMine` isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves `IsMine` into the wallet module and for it to take a `CWallet` instead of `CKeyStore`. The test that used `IsMine` is also moved to the wallet tests.
This is first [prerequisites](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#ismine) for the wallet structure changes.
ACKs for commit e61de6:
MarcoFalke:
re-ACK e61de6306f (only change is rebase with git auto-merge)
meshcollider:
Very light code review ACK e61de6306f
Tree-SHA512: 1cb4ad12652aef7922ab7460c6d413e8b9d1855dca78c0a286ae49d5c0765bc7996c55f262c742001d434eb9bd4215dc2cc7aae1b371ee1a82d46b32c17e6341
Co-authored-by: MeshCollider <dobsonsa68@gmail.com>
5db506ba5943868cc2c845f717508739b7f05714 tests: Add option --valgrind to run nodes under valgrind in the functional tests (practicalswift)
Pull request description:
What is better than fixing bugs? Fixing entire bug classes of course! :)
Add option `--valgrind` to run the functional tests under Valgrind.
Regular functional testing under Valgrind would have caught many of the uninitialized reads we've seen historically.
Let's kill this bug class once and for all: let's never use an uninitialized value ever again. Or at least not one that would be triggered by running the functional tests! :)
My hope is that this addition will make it super-easy to run the functional tests under Valgrind and thus increase the probability of people making use of it :)
Hopefully `test/functional/test_runner.py --valgrind` will become a natural part of the pre-release QA process.
**Usage:**
```
$ test/functional/test_runner.py --help
…
--valgrind run nodes under the valgrind memory error detector:
expect at least a ~10x slowdown, valgrind 3.14 or
later required
```
**Live demo:**
First, let's re-introduce a memory bug by reverting the recent P2P uninitialized read bug fix from PR #17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have").
```
$ git diff
diff --git a/src/consensus/validation.h b/src/consensus/validation.h
index 3401eb64c..940adea33 100644
--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};
class TxValidationState : public ValidationState {
private:
- TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
+ TxValidationResult m_result;
public:
bool Invalid(TxValidationResult result,
const std::string &reject_reason="",
```
Second, let's test as normal without Valgrind:
```
$ test/functional/p2p_segwit.py -l INFO
2019-11-28T09:30:42.810000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__fc8q3qo
…
2019-11-28T09:31:57.187000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
…
2019-11-28T09:32:08.265000Z TestFramework (INFO): Tests successful
```
Third, let's test with `--valgrind` and see if the test fail (as we expect) when the unitialized value is used:
```
$ test/functional/p2p_segwit.py -l INFO --valgrind
2019-11-28T09:32:33.018000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_gtjecx2l
…
2019-11-28T09:40:36.702000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
2019-11-28T09:40:37.813000Z TestFramework (ERROR): Assertion failed
ConnectionRefusedError: [Errno 111] Connection refused
```
ACKs for top commit:
MarcoFalke:
ACK 5db506ba5943868cc2c845f717508739b7f05714
jonatack:
ACK 5db506ba5943868cc2c845f717508739b7f05714
Tree-SHA512: 2eaecacf4da166febad88b2a8ee6d7ac2bcd38d4c1892ca39516b6343e8f8c8814edf5eaf14c90f11a069a0389d24f0713076112ac284de987e72fc5f6cc3795
fa37e0a68bea65979f9f8f2e5258fe608acf2bdf test: Show debug log on unit test failure (MarcoFalke)
Pull request description:
Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system).
Fix that by printing the log on failure.
ACKs for top commit:
jamesob:
ACK fa37e0a68bea65979f9f8f2e5258fe608acf2bdf ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u))
Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
9743432034586385cfef87df4b377c255ed0cba8 Fix bug where duplicate PSBT keys are accepted (John L. Jegutanis)
Pull request description:
As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.
For example, the following PSBT, included in the test vectors,
contains a duplicate field:
```
// magic
70736274ff
// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00
// no inputs
// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
ACKs for top commit:
achow101:
ACK 9743432034586385cfef87df4b377c255ed0cba8
instagibbs:
code review ACK 9743432034
Tree-SHA512: 34f4b34c8e6561c6a6ab745cdd319f6687eac6f7cecc735c94035eeca8c5157e17a27f2ae853dbaa6634fcd5a8f4e1c6cc13d1ebd7e563459665d72bb147cc1e
fac86ac7b3ceac2f884412c7a9f4bd5bab5e3916 scripted-diff: Add missed copyright headers (Hennadii Stepanov)
6fde9d5e47fc9a1042b3fb68031eab5bf55e508d script: Update EXLUDE list in copyright_header.py (Hennadii Stepanov)
1998152f15fd2b0e83f5068c375a34feaf73db8c script: Add empty line after C++ copyright (Hennadii Stepanov)
071f2fc204f542c5a287ca8835115a2ee0bf2f50 script: Add ability to insert copyright to *.sh (Hennadii Stepanov)
Pull request description:
This PR improves `contrib/devtools/copyright_header.py` script and adds copyright headers to the files in `src` and `test` directories with two exceptions:
- [`src/reverse_iterator.h`](https://github.com/bitcoin/bitcoin/blob/master/src/reverse_iterator.h) (added to exceptions)
- [`src/test/fuzz/FuzzedDataProvider.h`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h) (added to exceptions)
On master 5622d8f3156a293e61d0964c33d4b21d8c9fd5e0:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
25 with zero copyrights
```
With this PR:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
2 with zero copyrights
```
~I am uncertain about our copyright policy with `build_msvc` and `contrib` directories content, so they are out of scope of this PR.~
ACKs for top commit:
MarcoFalke:
ACK fac86ac7b3ceac2f884412c7a9f4bd5bab5e3916
Tree-SHA512: d7832c4a7a1a3b7806119775b40ec35d7982f49ff0e6199b8cee4c0e0a36e68d51728b6ee9924b1c161df4bc6105bd93391b79d42914357fa522f499cb113fa8
8a6810d0d2759c69f63b53c48aa79e0cfdd88ffb Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2759c69f63b53c48aa79e0cfdd88ffb
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
* Merge #16291: gui: Stop translating PACKAGE_NAME
fa64b947bb3075ff8737656706b941af691908ab util: No translation of `Bitcoin Core` in the copyright (MarcoFalke)
fab85208f678ba1be53bdb73a73ce3c5c937d448 qt: Run «make translate» in ./src/ (MarcoFalke)
fabe87d2c923ab3a70b8cde2666a4d1cda8b22fb scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke)
fa5e9f157e568b7fbbea1482b393181f0733f2ba build: Stop translating PACKAGE_NAME (MarcoFalke)
Pull request description:
Generally the package name is not translated, but the package description is.
E.g. `GIMP` or `Firefox` are always called that way regardless of the system language. However, "`Firefox` webbrowser" or "`GIMP` image manipulation program" are translated.
ACKs for top commit:
hebasto:
ACK fa64b947bb3075ff8737656706b941af691908ab, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 626f811531182d0ba0ef1044930d32726773349bcb49b10261288a86ee6b80a183db30a87d817d5b0d501fad058ac22d6272311716b4f5a154f17c6f391a5a1a
* more
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
