7a2ecf16df Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr)
2952c46b92 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr)
d7092c392e QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr)
Pull request description:
Follow-up to #18192, not strictly necessary for 0.20
ACKs for top commit:
MarcoFalke:
re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰
jnewbery:
utACK 7a2ecf16df
Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff
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
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
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
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
6112a20982 test: replace (send_message + sync_with_ping) with send_and_ping (Jon Atack)
Pull request description:
This is a follow-up to faf1d04731 yesterday.
ACKs for top commit:
vasild:
utACK 6112a20
MarcoFalke:
ACK 6112a20982 🎞
Tree-SHA512: 749644ac9a1ef0e1aa6c3ac5e899eb3fa7fb9c0909352f922a80412df2bc0e539692a7757af550eff4d4914cbe57b0c75ce3948f569acc7a52852e91a55ad457
9eefc6e92f gui: Delete progress dialog instead of hidding it (João Barbosa)
ee9e88ba27 wallet: Handle duplicate fileid exception (João Barbosa)
Pull request description:
Handle the duplicate fileid exception thrown at `CheckUniqueFileid` in tow cases:
- when duplicate wallets are set on the command line - catch in `LoadWallets`;
- when a duplicate wallet is loaded dynamically - catch in `LoadWallet`.
Fixes#16776.
ACKs for top commit:
jonatack:
Re-ACK 9eefc6e92f no change since last review 68e0ff0e1f530c942721aab49cf67ffc07104628
hebasto:
re-ACK 9eefc6e92f
Tree-SHA512: 46e3c1cd6708b54e2d1c4973a74c8d5428822e04cecbc147cf200eb034efa385e867bd749c7c639020e83c9813fae8fed64a851bdd99abf60c33b07e0363f5d5
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.
fac3716b09 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 fac3716b09
Tree-SHA512: 12f771473c23e152dae4bfb201fadb2c3530cf439de64fea07d048734614543080a5d05c9c36e6e398c6a69c8279f609d34706599571814172a11bcfbea4a3b9
0055922958 test: add BIP37 'filterclear' test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the message type `filterclear` to the test framework and adds a simple test to `p2p_filter.py`, checking that arbitrary txs get relayed again after deleting the filter.
ACKs for top commit:
naumenkogs:
utACK 0055922958
Tree-SHA512: fe64e99a526865770707d8077b9968d3923f248045ec7fa56cd380dba85ac77a71a473d244ef3aede2fc0d287b8d7c6bc0156b6033b0c949c2058cc08e255697
83e1d92413 test: listsinceblock block height checks (Jon Atack)
Pull request description:
This is the second commit of #17535.
This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in #17437. It also cleans up code in the test function without changing or removing existing checks.
ACKs for top commit:
fjahr:
tested ACK 83e1d92413
ryanofsky:
Code review ACK 83e1d92413. Nice test improvements!
Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
fa15699969 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa15699969 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
dcda81c471 test: add coverage for script parse error in ParseScript (pierrenn)
Pull request description:
Follow up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18416#issuecomment-603966799
This adds a test case to raise the `script parse error` in `ParseScript`.
ACKs for top commit:
instagibbs:
utACK dcda81c471
Tree-SHA512: ae0ef2c00f34cee818c83582f190d5f4043159e922862f2b442b7b895b8ff3ca421533699247c12c367be77813b5205830a771cd47a18e8932807ccace2d6a1c
9ab14e4d21 Limit decimal range of numbers ParseScript accepts (pierrenn)
Pull request description:
Following up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18413#issuecomment-602966490, prevent the output of `atoi64` in the `core_read.cpp:ParseScript` helper to send to `CScriptNum::serialize` values wider than 32-bit.
Since the `ParseScript` helper is only used by the tool defined in `bitcoin-tx.cpp`, this only prevents users to provide too much unrealistic values.
ACKs for top commit:
laanwj:
ACK 9ab14e4d21
Tree-SHA512: ee228269d19d04e8fee0aa7c0ae2bb0a2b437b8e574356e8d9b2279318242057d51fcf39a842aa3afe27408d0f2d5276df245d07a3f4828644a366f80587b666
c3857c5fcb wallet: remove CreateTotalBumpTransaction() (Jon Atack)
4a0b27bb01 wallet: remove totalfee from createBumpTransaction() (Jon Atack)
e347cfa9a7 rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack)
bd05f96d79 test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack)
a6d1ab8caa test: update bumpfee testing from totalFee to fee_rate (Jon Atack)
Pull request description:
Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it.
ACKs for top commit:
laanwj:
ACK c3857c5fcb
Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00
d056df033a Replace std::to_string with locale-independent alternative (Ben Woosley)
Pull request description:
Addresses #17866 following practicalswift's suggestion:
https://github.com/bitcoin/bitcoin/issues/17866#issuecomment-584287299
~Used ::ToString to avoid aliasing issues. Left uses in QT and test.~
ACKs for top commit:
practicalswift:
ACK d056df033a
laanwj:
ACK d056df033a
Tree-SHA512: 9e6966a9cdd14f4a1a40d9f0fa7c402aed22b2f1ad8681708e22b050d51a91c5d62220a9ec4c425be2d57acf5c964fca87a5e981b5cbff048bc3b6720dae92b7
fa92af5af3 ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01febeaf test: Remove ci timeout restriction in test_runner (MarcoFalke)
Pull request description:
Also revert commit 0a4912e46a, because some tests take too long for this to be useful anymore.
Top commit has no ACKs.
Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
