b84e776fd166ac422ac4a0dcdc1c6f7fd921bf70 wallet_importmulti: use addresses of the same type as being imported (Andrew Chow)
Pull request description:
When constructing an import from the solving data of an address, make sure that the original address is the same type as the one that will be imported.
See also: https://github.com/bitcoin/bitcoin/pull/17374#issuecomment-550036931
Part of #17261
ACKs for top commit:
Sjors:
Code review ACK b84e776
meshcollider:
Tested re-ACK b84e776fd166ac422ac4a0dcdc1c6f7fd921bf70
Tree-SHA512: 53c49c63af8cbade0116a62beddc77df1a411d8ed76571c3053f6aff096f41a5325421a188bab3dcacfda69bb28fdff6ba921ddd80f29c4abbadb3b58fda884c
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
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
fa2ec9f4518c1da3ab3592537c6cd204693a6069 fuzz: Bump timeout in test_runner to accomodate for slow arm64 CPUs (MarcoFalke)
fa6e01b2f3cd8523a95bd1906ac5e62ee3cbf319 ci: Use clang-8 for fuzzing to run on aarch64 ci systems (MarcoFalke)
Pull request description:
Ubuntu bionic clang is clang version 6, which does not come with libfuzzer. So the ci system breaks down when run on aarch64.
Fix that by using clang-8
For reference, the previous error on my ci system was:
```
/usr/bin/ld: cannot find /usr/lib/llvm-6.0/lib/clang/6.0.0/lib/linux/libclang_rt.fuzzer-aarch64.a: No such file or directory
ACKs for top commit:
laanwj:
ACK fa2ec9f4518c1da3ab3592537c6cd204693a6069
Tree-SHA512: 4954dbc36c444d1ae145290115eea6291753c9810c92003ab8d75433c3fe3bfee439d3a99dc394418275527157a8b89f04038c8b16e08c69ec9ded50fb869e70
38516f9078497d3e8456d48300eca40c35a281f2 Fix input size assertion in wallet_bumpfee.py (Gregory Sanders)
Pull request description:
I was investigating a curious error for https://github.com/bitcoin/bitcoin/pull/17290 and realized that this check should have caught that error earlier in the test.
The loop is intended to ensure that only a single input exists the entire time until the change output disappears, a single additional bump occurs, then it leaves the loop.
Top commit has no ACKs.
Tree-SHA512: 1d2d6ef535ec2c55f516ee5de11352386ceac6bedaabc6842229a486d9f28d35310ad5f57bfcc1f1e654fc397ecff29ec33256f9b3da897500b7e1635004b63a
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
To test the custom ancestor limit on node1 (passed by the argument
-limitancestorcount), we check for 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
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
Use -whitelist to speed up transaction relay.
The wallet_avoidreuse.py test is not intended to test transaction relay/timing,
so it should be fine to do this here.
This greatly reduces test run time variability and speeds up the test by 2-3
times on average, e.g. on my system from 20-30 seconds down to 8-10 seconds.
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
This was only added in c1dde3a949b36ce9c2155777b3fa1372e7ed97d8 to match
behaviour when `encryptwallet` would restart the node. It's not required
for the test (and slows things down).
When constructing an import from the solving data of an address,
make sure that the original address is the same type as the one that
will be imported.
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
4671fc3d9e669da8b8781f0cbefee43cb9acd527 Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08aff395dd813296bf99fd8ccc81bb27 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112199aa4e98d756039855cc561687c2e Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf79613f0e0f61bfd742ed265213ed280 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d144923f365cb1c2fad181eca15d1668692 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267f50c59c2eea1d46edaa5019a8ad5d8 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd1354f09e2d00bab29288db0d5665d0 Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de7f1b94428cdd1310777aa6a4c584e5 Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce294fe0d531770f540b1de00dc1d13f4b Make IsTrusted scan parents recursively (Jeremy Rubin)
Pull request description:
This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.
This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.
This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.
The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.
# Before `test_balance()`, we have had two nodes with a balance of 50
# each and then we:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 60 from node B to node A with fee 0.01
#
# Then we check the balances:
#
# 1) As is
# 2) With transaction 2 from above with 2x the fee
#
# Prior to #16766, in this situation, the node would immediately report
# a balance of 30 on node B as unconfirmed and trusted.
#
# After #16766, we show that balance as unconfirmed.
#
# The balance is indeed "trusted" and "confirmed" insofar as removing
# the mempool transactions would return at least that much money. But
# the algorithm after #16766 marks it as unconfirmed because the 'taint'
# tracking of transaction trust for summing balances doesn't consider
# which inputs belong to a user. In this case, the change output in
# question could be "destroyed" by replace the 1st transaction above.
#
# The post #16766 behavior is correct; we shouldn't be treating those
# funds as confirmed. If you want to rely on that specific UTXO existing
# which has given you that balance, you cannot, as a third party
# spending the other input would destroy that unconfirmed.
#
# For example, if the test transactions were:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 10 from node B to node A with fee 0.01
#
# Then our node would report a confirmed balance of 40 + 50 - 10 = 80
# BTC, which is more than would be available if transaction 1 were
# replaced.
The release notes have been updated to note the new behavior.
ACKs for top commit:
ariard:
Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
fjahr:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527
ryanofsky:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
promag:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527.
Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
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
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
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.
c98bd13e675fbf5641ed64d551b63aaf55a1a8e9 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)
Pull request description:
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
ACKs for top commit:
practicalswift:
ACK c98bd13e675fbf5641ed64d551b63aaf55a1a8e9 -- diff looks correct
Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
8d8e5a79d09d9027e5b68fecfc713e7b135320ba test: use default address type (bech32) for wallet_bumpfee tests (Sebastian Falbesoner)
Pull request description:
The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads to smaller transaction sizes, needing adaption of some constants in the following test cases:
- `test_dust_to_fee()`: adaption of dust calculation (p2wpkh spend estimate of 67 is taken from `src/policy/policy.cpp:GetDustThreshold()`)
- `test_maxtxfee_fails()`: lowering `-maxtxfee` setting to trigger fail
Top commit has no ACKs.
Tree-SHA512: b4163700d56c11955f811bc5fe6edaf7aec69931d7db741c03b055fb518bb9825c031fb931c513b37a1968085cb8c2f263adf664b357aff8ee42795fd0f88d2d
