* src/evo/evodb.cpp:57:29: warning: Assert statement calls a function which may have desired side effects: 'IsClean'. [assertWithSideEffect]
* src/llmq/quorums.cpp:635:37: note: Null pointer dereference
src/llmq/quorums.cpp:635:37: warning: Either the condition 'pFrom==nullptr' is redundant or there is possible null pointer dereference: pFrom. [nullPointerRedundantCheck]
src/llmq/quorums.cpp:636:81: note: Assuming that condition 'pFrom==nullptr' is not redundant
* fix a bunch of cppcheck warnings
* cppcheck: run on many more files. Enable all checks except a few ignored ones.
ignored
```
"Consider using std::transform algorithm instead of a raw loop."
"Consider using std::accumulate algorithm instead of a raw loop."
```
* ci: build specific version of cppcheck instead of install from apt
* ci: use cppcheck 2.4, remove commented out line, fix symlink
cppcheck 2.6 is latest, however causes issues
```
src/spork.cpp:135:51: warning: Analysis failed. If the code is valid then please report this failure. [cppcheckError]
```
cppcheck 2.5 appears to get into an infinite loop
* no need to check presence before insertion
* use if-init, remove redundant check
* remove redundant check
* don't remove cmake? fix macOs depends build?
* cppcheck: one per line, alphabetize
* remove duplicate cmake install
568a1d72619371a45b14a8356d3f80bd0c0efabc fix ecdsa verify in test framework (Stepan Snigirev)
Pull request description:
This PR fixes a small bug in the test framework in `verify_ecdsa` function.
`r` in ecdsa signature is modulo curve order, so if the point `R` calculated during verification has x-coordinate that is larger than the curve order, the verification will fail in the test framework but pass in libsecp256k1.
Example (all in hex):
public key: `0289d889551598a0263746c01e5882ccf9b7dc4ca5a37108482c9d80de40e0a8cf`
der signature: `3006020104020104` (r = 4, s = 4)
message: `3232323232323232323232323232323232323232323232323232323232323232`
libsecp256k1 returns `true`, test framework returns `false`.
ACKs for top commit:
sipa:
utACK 568a1d72619371a45b14a8356d3f80bd0c0efabc
Tree-SHA512: 9e9c58498f10085d2ad85e95caff6c92793799d2a40696ef43febcd7d313c8c3d5ecec715ca903cbb8432a8a96bd0065d86d060966d4ee651c3871ce16c252bf
f471a3be00c2b6433b8c258b716982c0539da13f scripted diff: Improve invalid vout value rpc error message (Nima Yazdanmehr)
Pull request description:
Since the `vout` value can start at `0`, the error message for *negative* values can be improved to something like: `vout cannot be negative`.
ACKs for top commit:
fanquake:
ACK f471a3be00c2b6433b8c258b716982c0539da13f
promag:
Code review ACK f471a3be00c2b6433b8c258b716982c0539da13f.
Tree-SHA512: fbdee3d0ddd5b58eb93934a1217b44e125a9ad39e672b1f35c7609c6c5fcf45ae1b731d3d6135b7225d98792dbfc34a50907b8c41274a5b029d7b5c59f886560
fa1433ac1be8481f08c1a0a311a6b87d8a874c6a rpc: Remove special case for unknown service flags (MarcoFalke)
Pull request description:
The special case to return a bit as an integer is clumsy and undocumented. Probably also irrelevant because there shouldn't currently be a non-misbehaving client that connects to Bitcoin Core and advertises an unknown service flag.
Thus, simply remove the code.
ACKs for top commit:
laanwj:
ACK fa1433ac1be8481f08c1a0a311a6b87d8a874c6a
Tree-SHA512: 942de6a577a9ee076ce12c92be121617640d53ee8c3424064c45a30a7ff789555d3722a4203670768faf81da2a40adfed3ec5cdeb5da06f04be81ddb53b9db7e
fad21a1a7aa8804f4699e5821f074f5d3845c78b test: Explain that a bug should be filed when the test fail (MarcoFalke)
Pull request description:
Without a bug report it is harder to fix the issue
ACKs for top commit:
hebasto:
ACK fad21a1a7aa8804f4699e5821f074f5d3845c78b, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
fanquake:
ACK fad21a1a7aa8804f4699e5821f074f5d3845c78b
Tree-SHA512: db194e8f8c0f07b2f4c9ef27e456510959f89da69435cee71605d720e0ad06f18700973f5af25ea31a190b933eb35f2743f014878aa3f8293500e06b4907ebbd
fa8b9b5d1f48ad95eecf47ebbd7bf374777fc621 test: Fix intermittent failure in wallet_importmulti (MarcoFalke)
Pull request description:
The wallet is async, so after generating a block, we must call `syncwithvalidationinterfacequeue`. Otherwise the timestamp will be of the previous block.
https://travis-ci.org/github/bitcoin/bitcoin/jobs/677685073#L2648
ACKs for top commit:
promag:
ACK fa8b9b5d1f48ad95eecf47ebbd7bf374777fc621.
Tree-SHA512: c21f9912aabbe22019d4ac9d0da06d6e46ef7f2a84d2781110e04c9836eb0ecf90a22cf2bae7f608be611670d17b20600135d1c5e5404aa1e762839816285fb4
faf6f156ffd1a8ed1aed047428d791a8c13c162b test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
Pull request description:
The wallet rebroadcast functionality learns about new blocks via the validation interface queue. To avoid test failures such as https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31119387#L466 , we can sync with the queue before advancing the test.
ACKs for top commit:
jonatack:
ACK faf6f156 this makes sense; the fix was previously added to mempool_persist.py and wallet_zapwallettxes.py in #12217 and to wallet_balance.py in #16302. It is also used in src/test/validation_block_tests.cpp (processnewblock_signals_ordering) and src/bench/wallet_balance.cpp.
Tree-SHA512: d72fd4b597b669d8111007902b523e946712913cd6eea6f9a695b0f04ecbe2321d05019873af999a95b9e0aa0f5c140a17109b37503723e40c9eab24ec358eb7
7d263571bee8c36fbe3c854b69c6f31cf1ee3b9b rpc: require second argument only for scantxoutset start action (Andrew Chow)
Pull request description:
It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0:
```
<belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
<belcher> im on regtest, in case its important
<harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there.
<harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
<jonatack> Same for me as well
<harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.
```
As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them.
It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments.
To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case.
ACKs for top commit:
laanwj:
ACK 7d263571bee8c36fbe3c854b69c6f31cf1ee3b9b
promag:
ACK 7d263571bee8c36fbe3c854b69c6f31cf1ee3b9b.
Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526
a67352161c68fea9764cc31aff199f112d8572c6 test: skip tool_wallet test when bitcoin-wallet isn't compiled (fanquake)
e9277baed64e1d4054a102e40b39a9aed7839c2f test: skip wallet_listreceivedby test when the cli isn't compiled (fanquake)
621d398750d9f5ce3e7ec75ccb160b3534dcc436 test: skip bitcoin_cli test when the cli isn't compiled (fanquake)
Pull request description:
Don't try and run the `interface_bitcoin_cli.py` test when `bitcoin-cli` isn't available.
```bash
stdout:
2019-11-17T01:51:41.623000Z TestFramework (INFO): Initializing test directory /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20191116_205141/interface_bitcoin_cli_0
2019-11-17T01:51:41.890000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/Users/michael/github/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
self.run_test()
File "/Users/michael/github/bitcoin/test/functional/interface_bitcoin_cli.py", line 18, in run_test
cli_response = self.nodes[0].cli("-version").send_cli()
File "/Users/michael/github/bitcoin/test/functional/test_framework/test_node.py", line 528, in send_cli
process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
File "/Users/michael/.pyenv/versions/3.5.6/lib/python3.5/subprocess.py", line 676, in __init__
restore_signals, start_new_session)
File "/Users/michael/.pyenv/versions/3.5.6/lib/python3.5/subprocess.py", line 1289, in _execute_child
raise child_exception_type(errno_num, err_msg)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/michael/github/bitcoin/src/bitcoin-cli'
```
Top commit has no ACKs.
Tree-SHA512: de27513a615d9d21271a0948e012c3209351e7374efd19bfa1bb9cda77e8fffe15d99e3424e4dbfa8cf826084f8af1670726f4703bd2b6093e7d37df4bea64f0
4412a59bfe8228698e5b5bbe8bb21c8e8a70d357 qa: Remove race between connecting and shutdown on separate connections (João Barbosa)
Pull request description:
Fixes the error https://github.com/bitcoin/bitcoin/pull/14670#issuecomment-447255352 reported by @ken2812221.
There is a race between RPC stop and another concurrent call in the test framework. The connection must be established and the command `waitfornewblock` running before calling `stop`.
See also https://github.com/bitcoin/bitcoin/pull/14670#issuecomment-447304513.
Tree-SHA512: 77feb8628d3b9c025ec0cf83565d4d6680cad4fb182fc93a65df8b573f3e799ba4c44e06d9001dd8a375ca0b1ee17f10e66c3902b6256d0ae2acbc64539185d7
a0ac15459a0df598e1ee1fd36a3899a129cecaeb doc: Add getrpcinfo release notes (João Barbosa)
251a91c1bf245b3674c2612149382a0f1e18dc98 qa: Add tests for getrpcinfo (João Barbosa)
d0730f5ce475e5a84da7c61fe79bcd6ed24d693e rpc: Add getrpcinfo command (João Barbosa)
068a8fc05f8dbec198bdc3fe46f955d8a5255303 rpc: Track active commands (João Barbosa)
bf4383277d6761cc5b7a91975752c08df829af72 rpc: Remove unused PreCommand signal (João Barbosa)
Pull request description:
The new `getrpcinfo` command exposes details of the RPC interface. The details can be configuration properties or runtime values/stats.
This can be particular useful to coordinate concurrent functional tests (see #14958 from where this was extracted).
Tree-SHA512: 7292cb6087f4c429973d991aa2b53ffa1327d5a213df7d6ba5fc69b01b2e1a411f6d1609fed9234896293317dab05f65064da48b8f2b4a998eba532591d31882
effe81f750 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
bb8ae2c419 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)
Pull request description:
And use it to fix a race condition in mempool_persist.py:
https://travis-ci.org/Empact/bitcoin/jobs/487577243
Since e.g. getrawmempool returns errors based on this status, this
enables users to test it for readiness.
Fixes#12863
ACKs for commit effe81:
MarcoFalke:
utACK effe81f750
jnewbery:
utACK effe81f7503d2ca3c88cfdea687f9f997f353e0d
Tree-SHA512: 74328b0c17a97efb8a000d4ee49b9a673c2b6dde7ea30c43a6a2eff961a233351c9471f9a42344412135786c02bdf2ee1b2526651bb8fed68bd94d2120c4ef86
e16b6a7188 rpc: Rename size to vsize in mempool related calls (Miguel Herranz)
Pull request description:
#13008 rebased on `master`, with release notes split out.
> In getmempoolancestors, getmempooldescendants, getmempoolentry and getrawmempool RPCs size returns the virtual transaction size as defined in BIP 141. Renaming it to vsize makes it consistent with returned value and other calls such as getrawtransaction.
>
> Related to #11218.
ACKs for commit e16b6a:
MarcoFalke:
re-utACK e16b6a71880052a6f7a368d8357901b0460abaef
jnewbery:
utACK e16b6a71880052a6f7a368d8357901b0460abaef
Tree-SHA512: ce95260fe7f280eacf4ff70bfffe02315c3a521b3b462a34e72a05b90733f40cc473319ac2df05d3e3c12cb7b1fbf2a1bbea632a8f979fff94207854cdbd494d
581c9be0d8bff46cd68bd6a3bf72f22d11c09aea test: speedup wallet_backup by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
approaches part of #16613 ("Functional test suite bottlenecks")
The majority of the test time is spent in `sync_mempools()` after sending to
addresses, i.e. the bottleneck is in relaying transactions. By whitelisting the
peers via `-whitelist`, the inventory is transmissioned immediately rather than
on average every 5 seconds, speeding up the test by at least a factor of two:
before:
```
$ time ./wallet_backup.py
real 2m2.523s
user 0m6.093s
sys 0m2.454s
```
with this PR:
```
$ time ./wallet_backup_with_whitelist.py
real 0m36.570s
user 0m5.365s
sys 0m1.696s
```
Note that the test is not deterministic (the `sendtoaddress` RPC in function
`one_send()` is executed with a probability of 50%), hence the times could vary
between individual runs.
ACKs for top commit:
MarcoFalke:
ACK 581c9be0d8bff46cd68bd6a3bf72f22d11c09aea, this test is testing the backup behaviour, not the tx relay behaviour
fanquake:
ACK 581c9be0d8bff46cd68bd6a3bf72f22d11c09aea
Tree-SHA512: d016f39cdb85501e17a74a4c4db5a9f7404baa76fbcc3675a34d3cd7bf03d7a4cb4fd3e5f17cb0597248120bb5ac8b15d3db7663007b76b010902be72954bde0
bdd6a4fd5da44c2575be9195ecb4213a13e74511 qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
fc0c410d6e19dd8e3abbc9b0fc13c836e6678750 rpc: Improve scantxoutset response and help message (João Barbosa)
Pull request description:
The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.
The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.
ACKs for top commit:
laanwj:
ACK bdd6a4fd5da44c2575be9195ecb4213a13e74511
Tree-SHA512: 6bb7c3464b19857b756b8bc491ab7c58b0d948aad8c005b26ed27c55a1278f5639217e11a315bb505b4f44ebe86f413068c1e539c8a5f7a4007735586cc6443c
662d1171d9e29964b039ba4c5bc8a2304426c003 Add option to create an encrypted wallet (Andrew Chow)
Pull request description:
This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase.
This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.
ACKs for commit 662d11:
laanwj:
utACK 662d1171d9e29964b039ba4c5bc8a2304426c003
jnewbery:
Looks great. utACK 662d1171d9e29964b039ba4c5bc8a2304426c003
Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a
7687f7873 [wallet] Support creating a blank wallet (Andrew Chow)
Pull request description:
Alternative (kind of) to #14938
This PR adds a `blank` parameter to the `createwallet` RPC to create a wallet that has no private keys initially. `sethdseed` can then be used to make a clean wallet with a custom seed. `encryptwallet` can also be used to make a wallet that is born encrypted.
Instead of changing the version number as done in #14938, a wallet flag is used to indicate that the wallet should be blank. This flag is set at creation, and then unset when the wallet is no longer blank. A wallet becomes non-blank when a HD seed is set or anything is imported. The main change to create a blank wallet is primarily taken from #14938.
Also with this, the term "blank wallet" is used instead of "empty wallet" to avoid confusion with wallets that have balance which would also be referred to as "empty".
This is built on top of #15225 in order to fix GUI issues.
Tree-SHA512: 824d685e11ac2259a26b5ece99c67a7bda94a570cd921472c464243ee356b7734595ad35cc439b34357135df041ed9cba951e6edac194935c3a55a1dc4fcbdea
42a5e912ee4e91a5191d659588f0605e1ada2f33 [mempool] log correct messages when CPFP fails (John Newbery)
Pull request description:
Fixes a logging issue introduced in #15681
ACKs for top commit:
laanwj:
ACK 42a5e912ee4e91a5191d659588f0605e1ada2f33 (+utACK from bluematt that isn't registered because it has no commit id)
Tree-SHA512: ff5f423cc4d22838eea00c5b1d39ceda89cd61474c72f256a97c698eb0ec3f2156a97139f537669376132902c1e3943bf84c356a4b98a9a306b4ec57302c2761
50cede3f5a4d4fbfbb7c420b94e661a6a159bced [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo)
Pull request description:
This implements the proposed policy change from [1], which allows
certain classes of contract protocols involving revocation
punishments to use CPFP. Note that some such use-cases may still
want some form of one-deep package relay, though even this alone
may greatly simplify some lightning fee negotiation.
[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
ACKs for top commit:
ajtowns:
ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced -- looked over code again, compared with previous commit, compiles, etc.
sdaftuar:
ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced
ryanofsky:
utACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.
Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c
43e7d576f590e90ad7d1ba3d550671a7958f1188 doc: Improve test READMEs (Fabian Jahr)
Pull request description:
General improvements on READMEs for unit tests and functional tests:
- Give unit test readme a headline
- Move general information on `src/test` folder to the top
- Add information on logging and debugging unit tests
- Improve debugging and logging information in functional testing
- Include all available log levels in functional tests
ACKs for top commit:
laanwj:
ACK 43e7d576f590e90ad7d1ba3d550671a7958f1188
Tree-SHA512: 22b27644992ba5d99a885cd51b7a474806714396fcea1fd2d6285e41bdf3b28835ad8c81449099e3ee15a63d57b3ab9acb89c425d9855ed1d9b4af21db35ab03
fa69588537bc91c0aedbc89ef1760d89cbffad75 test: Make PORT_MIN in test runner configurable (MarcoFalke)
Pull request description:
This is needed when some ports in the port range are used by other processes. Note that simply assigning the ports dynamically does not work:
* We spin up several nodes per test (each node gets its own port)
* We run several tests in parallel
So to avoid nodes from different tests colliding on ports, the port assignment must be deterministic (can not be dynamic).
Fixes: #10869
ACKs for top commit:
practicalswift:
ACK fa69588537bc91c0aedbc89ef1760d89cbffad75 -- diff looks correct
promag:
ACK fa69588537bc91c0aedbc89ef1760d89cbffad75.
Tree-SHA512: e79adb015e7de79064e2d14336c38bc9672bd779ad6c52917721897e73f617c39d32c068a369c26670002a6c4ab95a71ef3a6878ebdd9710e02f410e2f7bcd14
96299a9d6c0a6b9125a58a63ee3147e55d1b086b Test: Move common function assert_approx() into util.py (fridokus)
Pull request description:
To reduce code duplication, move `assert_approx` into common framework `util.py`.
`assert_approx()` is used in two functional tests.
ACKs for top commit:
theStack:
ACK 96299a9
practicalswift:
ACK 96299a9d6c0a6b9125a58a63ee3147e55d1b086b -- DRY is good and diff looks correct
fanquake:
ACK 96299a9d6c0a6b9125a58a63ee3147e55d1b086b - thanks for contributing 🍻
Tree-SHA512: 8e9d397222c49536c7b3d6d0756cc5af17113e5af8707ac48a500fff1811167fb2e03f3c0445b0b9e80f34935f4d57cfb935c4790f6f5463a32a67df5f736939
fadfd844de8c53034a97dfa6f771ffe9f523fba2 test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2280af4bcbcfffff275aaf8dd125929 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a91b3f603881655d3980c29af09852b test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f517838e5b9f778bf6b5a9c8d561e857 test: Reformat python imports to aid scripted diff (MarcoFalke)
Pull request description:
By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).
This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.
Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.
Historic background:
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
ACKs for top commit:
laanwj:
ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2
jonasschnelli:
utACK fadfd844de8c53034a97dfa6f771ffe9f523fba2 - more of less a cleanup PR.
promag:
Tested ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2, ran extended tests.
Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
0c62e3aa73839e97e65a3155e06a98d84b700a1e New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2ad68719415e9c54a981441052da072 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73839e97e65a3155e06a98d84b700a1e, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
fae961de6be3e2ab9793d437079651541e219e71 test: Establish only one connection between nodes in rpc_invalidateblock (MarcoFalke)
Pull request description:
Headers and block sync should eventually converge to the same result, regardless of whether the peers treat each other as "inbound" or "outbound".
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
Thus remove the `connect_nodes_bi` workaround from the rpc_invalidateblock test.
Conveniently, this also closes#16453. See https://github.com/bitcoin/bitcoin/issues/16444#issuecomment-514801708 for rationale
ACKs for top commit:
laanwj:
ACK fae961de6be3e2ab9793d437079651541e219e71
Tree-SHA512: b3614c66a205823df73f64d19cacfbec269beb5db52ff79004d746e17d7c0dfb43ab9785fdddc97e2a76fe76286c8c605b34df3dda4a2bf5be035f01169ae89a
7195fa792fcc19e9c064c4e38814c3b46a210b34 test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack)
3bf2b3a37bbd550491d124b77fd7c1b2a7969f66 test: Split tool_wallet.py test into subtests (Jon Atack)
1eb13f09a9d8c2c7dc69f4cdf1b1ccf632543aa0 test: Add log messages to test/functional/tool_wallet.py (Jon Atack)
Pull request description:
This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608 and serve as a benchmark for fixing the issue:
- Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.
- Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.
Goals:
1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
2. Add info log messages as there are currently none in the test file.
3. Split the tests out to separate functions as per review feedback.
Thanks to Marco Falke for pointing me in the right direction.
ACKs for top commit:
laanwj:
code review ACK 7195fa792fcc19e9c064c4e38814c3b46a210b34
Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b
faa1e0fb1712b1f94334e42794163f79988270fd qt: test: Create at most one testing setup (MarcoFalke)
Pull request description:
It is assumed that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).
This assumption is violated in the GUI tests, as a testing setup is created as the first step of the `main` function and then (sometimes) another one for the following test cases.
So, the gui tests create two testing setups:
* `BasicTestingSetup` in `main` (added in fa4a04a5a942d582c62773d815c7e1e9897975d0)
* a testing setup for individual test cases
Avoid that by destructing the testing setup in main after creation and then move the explicit `ECC_Stop` to the only places where it is needed (before and after `apptests`).
ACKs for top commit:
laanwj:
code review ACK faa1e0fb1712b1f94334e42794163f79988270fd
Tree-SHA512: b8edceb7e2a8749e1de3ea80bc20b6fb7d4390bf366bb9817206ada3dc8669a91416f4803c22a0e6c636c514e0c858dcfe04523221f8851b10deaf472f107d82
* test: Add BRR and DAT unit tests
* test: Drop feature_block_reward_reallocation.py
* change copyright
* remove trivially removable includes
* use constexpr, remove empty statement
* Don't use BOOST_ASSERT, fix bug?
Not sure if this was a bug, as it does an assignment. If this isn't a bug please add a comment / explanation
```
BOOST_ASSERT(::ChainActive().Tip()->nVersion = 536870912);
```
* deduplicate all the things (also test all activation periods)
* use try_emplace, and remove some tempararies
* update threshold to be inline with dynamic
* explicitly include map, vector, remove now unneeded base58.h
* remove unused param, and replace raw loop with range loop
* re: Don't use BOOST_ASSERT, fix bug?
* Make TestChain<smth>Setup in dynamic_activation_thresholds_tests more general
* Specify min level activation tests correctly
Co-authored-by: Pasta <pasta@dashboost.org>
bfe1ba2f5b36056e0c41edf8206b93d3d83098df rel-builds: Specify core.abbrev for git-rev-parse (Carl Dong)
27e63e01cce368d67092de8f0c736927d6f6aa69 build: Accomodate makensis v2.x (Carl Dong)
1f2c39a30e0f82046c7aecddfda3eb99cb536816 guix: Remove logical cores requirement (Carl Dong)
a4f6ffa71e335d4b2a6bf525b7f416968f9cd9f7 lint: Also enable source statements for non-gitian (Carl Dong)
d256f91cb1b0d6ff5170106b99b0266cbe51f5a2 rel-builds: Directly deploy win installer to OUTDIR (Carl Dong)
fa791da02f9684e3fd554b687fb692ae6a23d65a nsis: Specify OutFile path only once (Carl Dong)
14701604d0904bc5bbf1c67de08f8ee6d3215523 guix: Expose GIT_COMMON_DIR in container as readonly (Carl Dong)
f5a6ac4f48b18f93050d77bcb23f9cf45ec34647 guix: Make source tarball using git-archive (Carl Dong)
395c1137f630dc495ffb2752a23bc1dfd470ee53 gitian: Limit sourced script to just assignments (Carl Dong)
Pull request description:
Based on: #18556
Related: https://github.com/bitcoin/bitcoin/pull/17595#discussion_r399728721
ACKs for top commit:
fanquake:
ACK bfe1ba2f5b36056e0c41edf8206b93d3d83098df - I agree with Carl, and am going to merge this. I'd like for Linux Guix builds to be working again, and we can rebase #18818.
Tree-SHA512: c87ada7e3de17ca0b692a91029b86573442ded5780fc081c214773f6b374a0cdbeaf6f6898c36669c2e247ee32aa7f82defb1180f8decac52c65f0c140f18674
2aa48edec0101f8a77a2189244fc62722ff7a123 refactor: Drop unused ${WRAP_DIR}/${HOST} directory (Hennadii Stepanov)
1362be044724bb49d785ca2e296a3b43343c1690 build: Drop make dist in gitian builds (Hennadii Stepanov)
Pull request description:
After the merge of #18331, the packaged source tarball is created by `git archive`, but the binaries are built from another one which is made by `make dist`.
With this PR the only source tarball, created by `git archive`, is used both for binaries building and for packaging to users.
Close#16588.
Close#18547.
As a good side-effect, #18349 becomes redundant.
**Change in behavior**
The following variables 1b151e3ffc/configure.ac (L2-L6)
are no longer used for naming of directories and tarballs.
Instead of them the gitian descriptors use a git tag (if available) or a commit hash.
---
Also a small refactor commit picked from #18404.
ACKs for top commit:
dongcarl:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123
MarcoFalke:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123
fanquake:
ACK 2aa48edec0101f8a77a2189244fc62722ff7a123 - I've had a quick look over this, and don't want to block merging if this actually gets as closer to finally having this all sorted out. Obviously we've still got #18741, and after speaking to Carl this morning, there will likely be even more changes after that (not Guix specific).
Tree-SHA512: d3b16f87e48d1790a3264940c28acd5d881bfd10f3ce94fb0c8a6af76d8039289d01e0cd4972adac49ae24362857251f6c1e5e09e3e9fbf636c10708b4015a7c
1ac454a3844b9b8389de0f660fa9455c0efa7140 Enable ShellCheck rules (Hennadii Stepanov)
Pull request description:
Enable some simple ShellCheck rules.
Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu.
For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166).
ACKs for top commit:
practicalswift:
utACK 1ac454a3844b9b8389de0f660fa9455c0efa7140
dongcarl:
utACK 1ac454a
fanquake:
ACK 1ac454a3844b9b8389de0f660fa9455c0efa7140
Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e
510c6532ba Extract ParseDescriptorRange (Ben Woosley)
Pull request description:
So as to be consistently informative when the checks fail, and
to protect against unintentional divergence among the checks.
ACKs for commit 510c65:
meshcollider:
Oh apologies, yes. Thanks :) utACK 510c6532ba
MarcoFalke:
utACK 510c6532bae9abc5beda1c126c945923a64680cb
sipa:
utACK 510c6532bae9abc5beda1c126c945923a64680cb
Tree-SHA512: b1f0792bfaa163890a20654a0fc2c4c4a996659916bf5f4a495662436b39326692a1a0c825caafd859e48c05f5dd1865c4f7c28092be5074edda3c94f94f9f8b
ca253f6ebf Make deriveaddresses use stop/[start,stop] notation for ranges (Pieter Wuille)
1675b7ce55 Use stop/[start,stop] notation in importmulti desc range (Pieter Wuille)
4566011631 Add support for stop/[start,stop] ranges to scantxoutset (Pieter Wuille)
6b9f45e81b Support ranges arguments in RPC help (Pieter Wuille)
7aa6a8aefb Add ParseRange function to parse args of the form int/[int,int] (Pieter Wuille)
Pull request description:
This introduces a consistent notation for RPC arguments in `scantxoutset`, `importmulti`, and `deriveaddresses`, either:
* `"range" : int` to just specify the end of the range
* `"range" : [int,int]` to specify both the begin and the end of the range.
For `scantxoutset`, this is a backward compatible new feature. For the two other RPCs, it's an incompatible change, but neither of them has been in a release so far. Because of that non-released reason, this only makes sense in 0.18, in my opinion.
I suggest this as an alternative to #15496, which only makes `deriveaddresses` compatible with `importmulti`, but not with the existing `scantxoutset` RPC. I also think `[int,int]` is more convenient than `{"start":int,"stop":int}`.
I realize this is technically a feature added to `scantxoutset` after the feature freeze. If desired, I'll drop the `scantxoutset` changes.
Tree-SHA512: 1cbebb90cf34f106786dbcec7afbf3f43fb8b7e46cc7e6763faf1bc1babf12375a1b3c3cf86ee83c21ed2171d99b5a2f60331850bc613db25538c38b6a056676
* instantsend: Avoid writing IS locks for unknown txes
* instantsend: Allow a competing tx into mempool if there is an islock waiting for it
* use try_emplace
* Hold cs_main while calling ResetBlockFailureFlags
fa8489a15511f61a372473927e73c34692bbec23 test: Add test for BIP30 duplicate tx (MarcoFalke)
77770d95e2838d7665fa8f621e9e83d79f9b3196 test: Properly serialize BIP34 coinbase height (MarcoFalke)
Pull request description:
This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in https://github.com/bitcoin/bitcoin/pull/16333#issuecomment-508604071)
We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request.
Also, add a commit to fix the BIP34 test failures reported in https://github.com/bitcoin/bitcoin/pull/14633#issue-227712540
ACKs for top commit:
laanwj:
Code review ACK fa8489a15511f61a372473927e73c34692bbec23
Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
d6b3640ac732f6f66a8cb6761084d1beecc8a876 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b5685eb6227d694572cb0f7bfbcc151b36 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2bb74171cd3a14b94f9882d8e19e9fb [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
fae91a09c453a9a95c382df765bd71e54698d5b2 test: Remove incorrect and unused try-block in assert_debug_log (MarcoFalke)
Pull request description:
This try block has accidentally been added by me in fa3e9f7627784ee00980590e5bf044a0e1249999.
It was unused all the time, but commit 6011c9d72d1df5c2cd09de6f85c21eb4f7eb1ba8 added a `return` in the finally block, muting all exceptions.
This can be tested by adding an `assert False` after any `with ...assert_debug_log...:` line.
ACKs for top commit:
laanwj:
ACK fae91a09c453a9a95c382df765bd71e54698d5b2
ryanofsky:
utACK fae91a09c453a9a95c382df765bd71e54698d5b2. I didn't know returning inside a `finally` block would cancel pending exceptions or return values, but I guess this makes sense and is a good thing to be aware of.
Tree-SHA512: 47ed0165062060e9af055a3e92f1a529cd41d00476bfad64e3cd141ae084d22f926a343bb1257717e164e15459a59ab66aed198c95d18bf780d8cb0b76aa3298
6011c9d72d1df5c2cd09de6f85c21eb4f7eb1ba8 QA: fix rpc_setban.py race (Jonas Schnelli)
Pull request description:
The new `rpc_setban.py` test failes regularly on CIs due to a race between injecting the ban and testing the log "on the other side".
The problem is, that the test immediately after the `addnode` command on node0 checks for the `dropped (banned)` entry on node1 (without giving some time).
Adding a 2 seconds sleep seems to solve the race (I guess there is no better event-driven delay).
Example of a failed test: https://bitcoinbuilds.org/index.php?ansilog=bf743910-103f-4b54-9a97-960c471061bd.log#l2906
Top commit has no ACKs.
Tree-SHA512: 680f8ea3e5ddb07e93f824f1aeff4a459e25e6c14715a39fc7670e50506d7cf25925348672c5c2d8ba3e1243ccf5effbc2456bcd094fb96868349f8d26e008f1
c4606b84329d760d7cee144bebe05807857edaae Add Travis check for single parameter constructors not marked "explicit" (practicalswift)
Pull request description:
Make single parameter constructors `explicit` (C++11).
Rationale from the developer notes:
> - By default, declare single-argument constructors `explicit`.
> - *Rationale*: This is a precaution to avoid unintended conversions that might
> arise when single-argument constructors are used as implicit conversion
> functions.
ACKs for top commit:
laanwj:
ACK c4606b84329d760d7cee144bebe05807857edaae
Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
9f9db39041 QA/mininode: Send all headers upfront in send_blocks_and_test to avoid sending an unconnected one (Luke Dashjr)
Pull request description:
While this doesn't currently trigger any problems, the network protocol does expect headers to be sent connectable in normal circumstances, and if too many are sent out of order will disconnect the peer.
ACKs for commit 9f9db3:
Tree-SHA512: 25b88718e4ba3d31aed2de7ece23fab9a0737fd6536c5e618ea8eb5a3a217dab0dffaebc4892df7993bcea7efb7c4fb5085fabebe99535b8f7fdde3c19df54ff
fa08c5cb99 test_runner: Move pruning back to extended (MarcoFalke)
Pull request description:
This reverts fafb55e2c2b257efd4e584f72adf62401c01d573, since the test is still too slow to run with asan enabled on a network hdd
ACKs for commit fa08c5:
jnewbery:
utACK fa08c5cb993f07fd4309f2a6bd9ef4696f07e24c
jonasschnelli:
utACK fa08c5cb993f07fd4309f2a6bd9ef4696f07e24c
Tree-SHA512: de16786b9d507a72210805c3e9eef360e5fc3d4bc3a81f7175b6cc70d1bc426cde7ac97bc0d1a0d4e0813067e1e251c2dd49256552cc6b52446b475251b7c32b
f402012cc fixup: Fix prunning test (João Barbosa)
97f517dd8 Fix RPC/pruneblockchain returned prune height (Jonas Schnelli)
Pull request description:
The help of `pruneblockchain` tells us that the return value is `Height of the last block pruned.`,... but the implementation naively returns the provided input `height` and therefore not respecting that pruning can't be done on all possible blockheight due to the fact that we only prune complete blockfiles (which combine multiple blocks).
This fixes the return value to actually return the correct prune height.
ACKs for commit f40201:
MarcoFalke:
ACK f402012ccfc596d7d94851dabbf386c278ff5335
Tree-SHA512: 88c910030ffb83196663e5ebebc29d036fcdbbb2ab266e4538991867924a61bacd8361c1fbf294a0ea7e02347ae183d792f10a10b8f6187e8a4c4c6e4124d7e6
* merge 15638: Move CheckTransaction from lib_server to lib_consensus
* merge 15638: Move policy settings to new src/policy/settings unit
* merge 15638: Move rpc utility methods to rpc/util
* merge 15638: Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp
* merge 15638: Move several units into common libraries
* merge 15638: Move wallet load functions to wallet/load unit
* merge 15638: Document src subdirectories and different libraries
* [build] Add several util units (cleanup)
* build: resolve missing declarations by re-specifying headers
e91f0a7af2 doc: Remove travis badge from readme (MarcoFalke)
Pull request description:
The readme(s) are shipped in the released source-code archive, in which case the travis badge is useless since it doesn't link to the travis result of the correct commit/tag/branch. GitHub embeds the correct links for each tag or commit that ci ran on, so we don't need this link in the readme.
ACKs for commit e91f0a:
hebasto:
ACK e91f0a7af2aec7d924f00da25c69d8f46e0dd33d
Tree-SHA512: 860435a58b38a9bd0bc62a1e74b3a63c138c9a2f09008a090d5ecc7fd86fa908d2e5eda41d16606507a238d9488fa5323405364a9556b670684a2e4838aead2d
b67978529a Add comments to Python ECDSA implementation (John Newbery)
8c7b9324ca Pure python EC (Pieter Wuille)
Pull request description:
This removes the dependency on OpenSSL for the interaction tests, by providing a pure-Python
toy implementation of secp256k1.
ACKs for commit b67978:
jnewbery:
utACK b67978529ad02fc2665f2362418dc53db2e25e17
Tree-SHA512: 181445eb08b316c46937b80dc10aa50d103ab1fdddaf834896c0ea22204889f7b13fd33cbcbd00ddba15f7e4686fe0d9f8e8bb4c0ad0e9587490c90be83966dc
3c3e31c3a4 [tests] Add wallet-tool test (João Barbosa)
49d2374acf [tools] Add wallet inspection and modification tool (Jonas Schnelli)
Pull request description:
Adds an offline tool `bitcoin-wallet-tool` for wallet creation and maintenance.
Currently this tool can create a new wallet file, display information on an existing wallet, and run the salvage and zapwallettxes maintenance tasks on an existing wallet. It can later be extended to support other common wallet maintenance tasks.
Doing wallet maintenance tasks in an offline tool makes much more sense (and is potentially safer) than having to spin up a full node.
Tree-SHA512: 75a28b8a58858d9d76c7532db40eacdefc5714ea5aab536fb1dc9756e2f7d750d69d68d59c50a68e633ce38fb5b8c3e3d4880db30fe01561e07ce58d42bceb2b
* Handle attempts to read non-existent records from isdb properly
* Do not reject blocks that conflict with islocks while still syncing
Otherwise you can stuck with no new blocks/headers which means you won't be able to verify new chainlocks that might override stored islocks
* Handle duplicates/conflicting islocks better
* More constness
3c5254a820c892b448dfb42991f6109a032a3730 Limit Python linting to files in the repo (practicalswift)
Pull request description:
Limit Python linting to files in the repo.
Before:
```
$ test/lint/lint-python.sh
not_under_version_control.py:195:9: F841 local variable 'e' is assigned to but never used
$
```
After:
```
$ test/lint/lint-python.sh
$
```
ACKs for commit 3c5254:
fanquake:
tACK 3c5254a820
Empact:
utACK 3c5254a820
Tree-SHA512: 68733494a5f2a7764eba938af227145f5ef9ddc9ff94840134e4d2684ca7b9a819fac491ec43102f93e5e9867373bfd46b46efc9d11528329b5ecb2282fffb16
0784af16ef remove parameters -addresstype=legacy in rpc_rawtransaction test (LongShao007)
a65dafa8f1 replace tx hash with txid in test rawtransaction (LongShao007)
Pull request description:
The transaction hash is different from txid for witness transactions, so we should use txid instead of hash.
ACKs for commit 0784af:
Tree-SHA512: 98b699eb5f25c3a603b11eb7072efe9bc69c0c0ecc7f996405de31bc45d92105970e09fd8e4f75b42a46498817f596d36d9b28eae7d24e63a4f2f2abfcee0eab
fa566b2601ee5a40bf814e529d7db253dacd28e7 test: Add missing sync_blocks to feature_pruning (MarcoFalke)
Pull request description:
Fixes#16537Fixes#16520
ACKs for top commit:
promag:
ACK fa566b2601ee5a40bf814e529d7db253dacd28e7.
jonatack:
ACK fa566b2601ee5a40bf814e529d7db253dacd28e7. These past few months I have been seeing intermittent failures with this test on master. Ran `(for i in {1..40}; do test/functional/feature_pruning.py -l=debug; done)` overnight with this change; no failures.
Tree-SHA512: 5181d5ea525f43ad09e1c8b9ae72e32219f483948854c6dc07dda24b790cbdf4012e586253a0e158a71a980d1ca9f5fdf06aafbe95b8ea3d9154ef2c8687395b
