fa2251df5e test: Use one node to avoid a race due to missing sync in rpc_signrawtransaction (MarcoFalke)
Pull request description:
Node 0 creates a transaction in a block, and node 1 sends a spending transaction without properly syncing the utxo set.
Fixes intermittent test failure in rpc_signrawtransaction
```
test 2020-04-01T00:14:03.400000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\projects\bitcoin\test\functional\test_framework\test_framework.py", line 112, in main
self.run_test()
File "C:\projects\bitcoin/test/functional/rpc_signrawtransaction.py", line 213, in run_test
self.witness_script_test()
File "C:\projects\bitcoin/test/functional/rpc_signrawtransaction.py", line 208, in witness_script_test
self.nodes[1].sendrawtransaction(spending_tx_signed['hex'])
File "C:\projects\bitcoin\test\functional\test_framework\coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "C:\projects\bitcoin\test\functional\test_framework\authproxy.py", line 141, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: bad-txns-inputs-missingorspent (-25)
```
Full log: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31864368
ACKs for top commit:
achow101:
ACK fa2251df5e
Tree-SHA512: 9450d216d9989d6c44028ae4b9818790cfb00796e0de22331422f775f74d697bb14ebae0e88dca20c6b641363780da384fe94c708e20fce9cfde929fb343b12f
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57a Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b1 Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)
Pull request description:
In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.
This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.
Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).
Fixing it is accomplished by:
* Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
* `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
* For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).
A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.
ACKs for top commit:
ryanofsky:
Code review ACK b5795a7886. Pretty clever and nicely implemented fix!
jonatack:
ACK b5795a7886 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4 and tested manually with rpc/cli
jnewbery:
Good fix. utACK b5795a788.
Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
7b8e15728d rpc: Fix rpcRunLater race in walletpassphrase (João Barbosa)
Pull request description:
Release locks before calling `rpcRunLater`.
Quick explanation: `rpcRunLater` leads to `event_free` which calls `event_del` which can wait for the event callback to finish if it's already running and that callback will try to lock wallet mutex - which is already locked in http thread.
Fixes#14995 , fixes#18482. Best reviewed with whitespace changes hidden.
ACKs for top commit:
MarcoFalke:
ACK 7b8e15728d, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞
ryanofsky:
Code review ACK 7b8e15728d. Just updated comment since last review
Tree-SHA512: 17874a2fa7b0e164fb0d7ee4cb7d59650275b8c03476fb291d60af8b758495457660d3912623fb26259fefe84aeba21c0a9e0c6467982ba511f19344ed5413ab
b224b4e7bd test: wallet_bumpfee assertion fixup (Jon Atack)
Pull request description:
Follow-up to #18516 to fix up an assertion as per suggested change in https://github.com/bitcoin/bitcoin/pull/18516#discussion_r404191587.
ACKs for top commit:
jnewbery:
ACK b224b4e7bd
Tree-SHA512: 4973bba73a67c1ffaf460921b3d454e9d66a40a67f73b7df742e24a0e389adba3946a3958a729391ee6bfa4ef844be759ebf71d14d788434c248e48a2bbe5bde
cd3b1569d9 Correctly compute redeemScript from witnessScript for signrawtransaction (Andrew Chow)
Pull request description:
`ParsePrevouts` uses `GetScriptForWitness` on the given witnessScript to find the corresponding redeemScript. This is incorrect when the witnessScript is either a P2PK or P2PKH script as it returns the corresponding P2WPK script instead of turning the witnessScript into a P2WSH script. Instead this should make the script a `WitnessV0ScriptHash` destination and get the script for that.
Test cases are also added. These will fail on master with a `redeemScript does not correspond to witnessScript`
Reported on [Bitcointalk](https://bitcointalk.org/index.php?topic=5236818.0)
ACKs for top commit:
MarcoFalke:
weak ACK cd3b1569d9, only checked that the test fails without the code change 🚰
instagibbs:
utACK cd3b1569d9
Tree-SHA512: afac671dbb52ce88bfb4a9ca3dd6065427ad52c9778d0549ad40e9286778f308adad24fb3b3c3089545d7f88c57c53d41224fd7a4bb207550eff2fe06600118f
25e03ba1ff test: relax bumpfee dust_to_fee txsize an extra vbyte (Jon Atack)
Pull request description:
Hopefully closes#18511 by allowing the transaction size to be 140-141 vbytes rather than strictly 141, and bumps with a slightly larger fee to ensure dust in the 140 vbyte case.
ACKs for top commit:
jnewbery:
utACK 25e03ba1ff
Tree-SHA512: 76a04e1ce090e48befe048ed6d412222d7f8bc951ff822850833061a0606b1bebc5289f7249737d3fb9aa26eb857f99543981037cea6babe3e578e2cfe8afcdb
d6815a2313 refactor: drop boost::signals2 in validationinterface (Russell Yanofsky)
Pull request description:
Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions.
Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: https://github.com/bitcoin/bitcoin/issues/18517, https://github.com/bitcoin/bitcoin/pull/18471
ACKs for top commit:
MarcoFalke:
ACK d6815a2313
laanwj:
ACK d6815a2313
hebasto:
re-ACK d6815a2313
promag:
ACK d6815a2313.
Tree-SHA512: 4fc0f14a8446e8616cc142af6c3d36815f3254525d30348ba8e4d4bc74c249a5a8c9bc119bdd1be7ebd7abe0b784bc0c5551a3e156a766890cb2fdd891a95919
c0c43ae147 test: skip backwards compat tests if not compiled with wallet (fanquake)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: d9975a1490e69134408b6b724cea26a6c1397d43f59850283b9e338ae38e00fefbcd868fb141e0a4bb55f02076690a99331f29cfa2d0fa66c165032b24a94081
0eeb0468e7 net: Hardcoded seeds update for 0.20 (Wladimir J. van der Laan)
Pull request description:
Update hardcoded seeds from http://bitcoin.sipa.be/seeds.txt.gz,
according to release process.
Output from makeseeds.py:
```
IPv4 IPv6 Onion Pass
1364173 244127 2454 Initial
1364173 244127 2454 Skip entries with invalid address
1129552 213117 2345 After removing duplicates
1129548 213117 2345 Skip entries from suspicious hosts
338216 191944 2249 Enforce minimal number of blocks
336851 188993 2189 Require service bit 1
6998 1520 150 Require minimum uptime
5682 1290 89 Require a known and recent user agent
5622 1279 89 Filter out hosts with multiple bitcoin ports
512 146 89 Look up ASNs and limit results per ASN and per net
```
Top commit has no ACKs.
Tree-SHA512: ce1c2cda18dd5bd22586a5283a0877f3bd890437cc29dc1d85452ba4a4d28032f591c8b37f3329e8e649556cf83750b6949a068fad76d1773853d93014609da0
9e071b0089 test: remove rapidcheck integration and tests (fanquake)
Pull request description:
Whilst the property tests are interesting, ultimately [rapidcheck](https://github.com/emil-e/rapidcheck) integration in this repository has not gained much traction. We have a limited number of tests, and they are rarely (if ever) run. Have discussed this with Chris Stewart.
ACKs for top commit:
practicalswift:
ACK 9e071b0089
Tree-SHA512: d0c12af3163382eee8413da420c63e39265a7b700709a05d518445832d45e049aed9508e32524db5228fe3ac114609a00b7bb890be047c07032e44a5ef4611e9
Remove inconsistency between functional and unit test environments and make it
possible to substitute bitcoin-qt and bitcoin-node in place of bitcoind in
python tests, or to link bitcoind against shared libraries.
autotools and automake changes to support multiprocess execution.
This adds a new --enable-multiprocess flag, and build configuration code to
detect libraries needed for multiprocess support. The --enable-multiprocess
flag builds new bitcoin-node and bitcoin-gui executables, which are updated in
https://github.com/bitcoin/bitcoin/pull/10102 to communicate across processes.
But for now they are functionally equivalent to existing bitcoind and
bitcoin-qt executables.
0ed2d8e07d test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the missing message type `filteradd` to the test framework and checks that the BIP37 implementation is not vulnerable to the "remote crash bug" [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700) anymore. Prior to v.0.8.4, it was possible to trigger a division-by-zero error on the following line in the function `CBloomFilter::Hash()`:
f0d6487e29/src/bloom.cpp (L45)
By setting a zero-length filter via `filterload`, `vData.size()` is 0, so the modulo operation above, called on any .insert() or .contains() operation then crashed the node. The test uses the approach of just sending an arbitrary `filteradd` message after, which calls `CBloomFilter::insert()` (and in turn `CBloomFilter::Hash()`) on the node. The vulnerability was fixed by commit 37c6389c5a (an intentional covert fix, [according to gmaxwell](https://github.com/bitcoin/bitcoin/issues/18483#issuecomment-608224095)), which introduced flags `isEmpty`/`isFull` that wouldn't call the `Hash()` member function if `isFull` is true (set to true by default constructor).
To validate that the test fails if the implementation is vulnerable, one can simply set the flags to false in the member function `UpdateEmptyFull()` (that is called after a filter received via `filterload` is constructed), which activates the vulnerable code path calling `Hash` in any case on adding or testing for data in the filter:
```diff
diff --git a/src/bloom.cpp b/src/bloom.cpp
index bd6069b..ef294a3 100644
--- a/src/bloom.cpp
+++ b/src/bloom.cpp
@@ -199,8 +199,8 @@ void CBloomFilter::UpdateEmptyFull()
full &= vData[i] == 0xff;
empty &= vData[i] == 0;
}
- isFull = full;
- isEmpty = empty;
+ isFull = false;
+ isEmpty = false;
}
```
Resulting in:
```
$ ./p2p_filter.py
[...]
2020-04-03T14:38:59.593000Z TestFramework (INFO): Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed
2020-04-03T14:38:59.695000Z TestFramework (ERROR): Assertion failed
[...]
[... some exceptions following ...]
```
ACKs for top commit:
naumenkogs:
utACK 0ed2d8e07d
Tree-SHA512: 02d0253d13eab70c4bd007b0750c56a5a92d05d419d53033523eeb3ed80318bc95196ab90f7745ea3ac9ebae7caee3adbf2a055a40a4124e0915226e49018fe8
691e2a7af7 build: create test_fuzz library from src/test/fuzz/fuzz.cpp (Harris)
Pull request description:
This PR creates a static library **libtest_fuzz.a** to speed up the compilation of fuzz tests. It is functionally similar to https://github.com/bitcoin/bitcoin/pull/17542
Fixes https://github.com/bitcoin/bitcoin/issues/18527
ACKs for top commit:
MarcoFalke:
ACK 691e2a7af7🦁
Tree-SHA512: 39d7d2731ca4370db518dbb969eb17ddbf9c030c3fe0dec0d04ff6578f24a128563fe5aced78300c92ce296623a7079fea5aea70619819a20c56fb34191f00ef
4670006762 test: remove redundant sync_with_ping after add_p2p_connection (Jon Atack)
Pull request description:
Now that #18247 is merged, these calls are redundant.
ACKs for top commit:
vasild:
utACK 4670006
Tree-SHA512: bdbfe8bcf9dbdde0a8115e3a62bfe359910798d7a3010d920ffca07049cb5f97bf8fb9b6f70079b0607105192b61a6d665774e59a2b678597b47ad6237595ad5
3dc8c012f0 test: remaining replacements of (send_message+sync_with_ping) with send_and_ping (Sebastian Falbesoner)
Pull request description:
This is a tiny follow-up PR to #18494, substituting the remaining occurences of `send_message(...)`/`sync_with_ping(...)` pairs with `send_and_ping(...)`, as suggested in the comment https://github.com/bitcoin/bitcoin/pull/18494#pullrequestreview-386418913. Thanks to jonatack and [MarcoFalke](https://github.com/bitcoin/bitcoin/pull/18494#issuecomment-608496342) for giving me the hint to do this follow-up.
ACKs for top commit:
practicalswift:
ACK 3dc8c012f0
Tree-SHA512: 44d64332933c23a7f59c0415e008ce1b2b2e07177f81cb9473b7c71558188f1c698e8973de5cc940280e4697f9553af852d9a42841304f82469673d1c8162852
facc332dc5 fuzz: Avoid running over all inputs after merging them (MarcoFalke)
Pull request description:
This cuts the time it takes to merge inputs by half
ACKs for top commit:
practicalswift:
ACK facc332dc5
Tree-SHA512: bb22992c463dd985d3b1e9b8908c591d0c8e620c38eba0a932d880f87133bfe4ca2036b166c4f79b92ddf7940f56c044e9cb8cc50309c74204df122b369c167d
fab32557f2 rpc: Make rpc documentation not depend on rpc args (MarcoFalke)
Pull request description:
This is required to host the documentation on a static resource (like a website or pdf)
ACKs for top commit:
emilengler:
utACK fab32557f2
promag:
ACK fab32557f2.
Tree-SHA512: 3ca2691c7fbd5f17c75df2887753da152f66521dcb7dee4c29af6339fdea011cecdd51f825b96bde9c6aaf82f4d915cbd5aacb52e4eae3898d9dbc216f627171
f32ab443a9 Bugfix: RPC: JSON null is not "None" (Luke Dashjr)
26dcf39581 Bugfix: RPC: Don't use a continuation elipsis after an elision elipsis (Luke Dashjr)
eca65caadc Bugfix: RPC: Add missing commas and correct indentation of explicit ELISION (Luke Dashjr)
Pull request description:
1. listsinceblock had a double ellipsis (elision + continuation); this looks ugly, just one is needed.
2. Elision ellipsis wasn't getting a comma, so was truncated to `".."` by comma-removal code.
3. Elision ellipsis was indented incorrectly (as if it was a subitem).
4. Similarly, type "none" would get truncated to `"Non"`, when it should really be `"null"` anyway.
ACKs for top commit:
MarcoFalke:
ACK f32ab443a9🐰
Tree-SHA512: 34e1c72673790ed11cdee838d64ea5e0ac498de19258df99d54b5322e003060123c65ad27ac2fd4729a1dfe52066a0629602a132b1ef85d4154affd99a065a3f
