[0.20] Final backports for rc2

fanquake commented at 4:37 am on May 14, 2020: member

Backports the following PRs to the 0.20 branch:

#18578: gui: Fix leak in CoinControlDialog::updateView
#18808: [net processing] Drop unknown types in getdata
#18814: rpc: Relock wallet only if most recent callback
#18878: test: Add test for conflicted wallet tx notifications
#18894: gui: Fix manual coin control with multiple wallets loaded
#18742: miner: Avoid stack-use-after-return in validationinterface
#18962: net processing: Only send a getheaders for one block in an INV
#18975: test: Remove const to work around compiler error on xenial

fanquake added the label Backport on May 14, 2020

fanquake added this to the milestone 0.20.0 on May 14, 2020

luke-jr commented at 4:52 am on May 14, 2020: member

#18902 is also needed (fixes a regression)

MarcoFalke commented at 11:29 am on May 14, 2020: member

#18902 is also needed (fixes a regression)

It looks like it needs rebase again. I guess we can push it back to rc3 (if there is one) or 0.20.1

laanwj commented at 3:02 pm on May 14, 2020: member

I must say the problem that #18902 solves is unclear to me. Does it affect the gitian build as we use to build the release, in practice? If so what is the user-visible issue? If not, If it’s some more theoretical or aesthetic I’d strongly prefer pushing it back to 0.20.1 (especially as it makes large changes to the gitian descriptor).

MarcoFalke commented at 3:08 pm on May 14, 2020: member

Ok, updated OP to remove #18287 and #18818 for now.

MarcoFalke commented at 4:30 pm on May 14, 2020: member

Open-Close to re-run ci. See #15847 (comment)

MarcoFalke closed this on May 14, 2020

MarcoFalke reopened this on May 14, 2020

MarcoFalke commented at 4:33 pm on May 14, 2020: member

If you feel strongly about the fuzzers being green, I can recommend #18757

laanwj commented at 4:35 pm on May 14, 2020: member

I think you need the following:

 0    squashme: remove const to work around compiler error on xenial
 1    
 2        test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
 3    
 4        const BlockValidationState state_dummy;
 5
 6diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp
 7index d2fc20e625afaff0f262cf6ee0437cdaccb2791a..ceba689e52970ee5fff32682742d77bc2df48c63 100644
 8--- a/src/test/validationinterface_tests.cpp
 9+++ b/src/test/validationinterface_tests.cpp
10@@ -23,7 +23,7 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
11     // Start thread to generate notifications
12     std::thread gen{[&] {
13         const CBlock block_dummy;
14-        const BlockValidationState state_dummy;
15+        BlockValidationState state_dummy;
16         while (generate) {
17             GetMainSignals().BlockChecked(block_dummy, state_dummy);
18         }

MarcoFalke commented at 4:38 pm on May 14, 2020: member

@laanwj Whoospie. I wonder how that compiled in the first place. though, this also needs to be fixed on master, so maybe should be merged there first.

laanwj commented at 4:42 pm on May 14, 2020: member

Wasn’t aware that this is a problem on master too. If so, see .#18975

MarcoFalke commented at 11:40 pm on May 14, 2020: member

Going to re-run once more to save the log. This just hit issue #16337

MarcoFalke closed this on May 14, 2020

MarcoFalke reopened this on May 14, 2020

gui: Fix itemWalletAddress leak when not tree mode

Github-Pull: #18578
Rebased-From: e8123eae40eb264bbb71007d0eb074901f0e2fe5

315ae14f3f

[net processing] ignore tx GETDATA from blocks-only peers

Co-Authored-By: John Newbery <john@johnnewbery.com>

Github-Pull: #18808
Rebased-From: 047ceac142246b5d51056a51dbf4645b31802be4

fb821731eb

[net processing] ignore unknown INV types in GETDATA messages

Co-Authored-By: John Newbery <john@johnnewbery.com>

Github-Pull: #18808
Rebased-From: e257cf71c851e25e1a533bf1d4296f6b55c81332

1e73d7248a

[test] test that an invalid GETDATA doesn't prevent processing of future messages

Co-Authored-By: John Newbery <john@johnnewbery.com>

Github-Pull: #18808
Rebased-From: 2f032556e08a04807c71eb02104ca9589eaadf1b

011532e380

[docs] Improve commenting in ProcessGetData()

Github-Pull: #18808
Rebased-From: 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7

a3fe458a1e

rpc: Add mutex to guard deadlineTimers

Github-Pull: #18814
Rebased-From: a2e6db5c4f1bb52a8814102b628e51652493d06a

ca4dac48c5

rpc: Relock wallet only if most recent callback

Github-Pull: #18814
Rebased-From: 9f59dde9740d065118bdddde75ef9f4e4603a7b1

251e321ad7

test: Add test for conflicted wallet tx notifications

Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

https://github.com/bitcoin/bitcoin/pull/9240 - accidental break
https://github.com/bitcoin/bitcoin/issues/9479 - bug report
https://github.com/bitcoin/bitcoin/pull/9371 - fix
https://github.com/bitcoin/bitcoin/pull/16624 - accidental break
https://github.com/bitcoin/bitcoin/issues/18325 - bug report
https://github.com/bitcoin/bitcoin/pull/18600 - potential fix

Github-Pull: #18878
Rebased-From: f963a680515eda66429b3d1537a7baf281ab9283

ed0afe8c1f

gui: Fix manual coin control with multiple wallets loaded

Github-Pull: #18894
Rebased-From: a8b5f1b133d4f23975a3fbfb7a415b17261466ee

ff4dc20750

test: Add unregister_validation_interface_race test

This commit is (intentionally) adding a broken test. The test is broken
because it registering a subscriber object that can go out of scope
while events are still being sent.

To run the broken test and reproduce the bug:
  - Remove comment /** and */
  - ./configure --with-sanitizers=address
  - export ASAN_OPTIONS=detect_leaks=0
  - make
  - while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no  ; do true; done

Github-Pull: #18742
Rebased-From: fab6d060ce5f580db538070beec1c5518c8c777c

37a620748b

miner: Avoid stack-use-after-return in validationinterface

This is achieved by switching to a shared_ptr.

Also, switch the validationinterfaces in the tests to use shared_ptrs
for the same reason.

Github-Pull: #18742
Rebased-From: 7777f2a4bb1f9d843bc50a4e35085cfbb2808780

cc7d34465b

test: Remove const to work around compiler error on xenial

Fix the following error in travis:

    test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor

    const BlockValidationState state_dummy;

Github-Pull: #18975
Rebased-From: 050e2ee6f28e7b31c167013be7313726e34084e9

cf2a6e2a39

[net processing] Only send a getheaders for one block in an INV

Headers-first is the primary method of announcement on the network. If a
node fell back sending blocks by inv, it's probably for a re-org. The
final block hash provided should be the highest, so send a getheaders
and then fetch the blocks we need to catch up.

Github-Pull: #18962
Rebased-From: 746736639e6d05acdb85c866d4c605c947d4c500

6161c94a61

fanquake force-pushed on May 15, 2020

fanquake commented at 0:14 am on May 15, 2020: member

Rebased to include https://github.com/bitcoin/bitcoin/commit/7d87ba0e022796b42d6c17b59bb735c94dd6e045 and https://github.com/bitcoin/bitcoin/commit/aa7c6858e6e480eb841195bdaf2ee0185f17f9a7, and added #18962 and #18975.

fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer

Github-Pull: #18757
Rebased-From: fdceb6328382ac0f9d643f9d42ba0509062d7d48

9a8fb4cf4b

fanquake commented at 4:22 am on May 15, 2020: member

If you feel strongly about the fuzzers being green, I can recommend #18757

I’ve now added this as well.

fanquake commented at 6:15 am on May 15, 2020: member

I’ve now added this as well.

script/script.h:332:35: runtime error: negation of -9223372036854775808 cannot be represented in type ‘int64_t’ (aka ’long’); cast to an unsigned type to negate this value to itself #0 0x55f522f90e9e in CScriptNum::serialize(long const&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./script/script.h:332:35

Looks like the fuzzers are not going to pass without even more backports (maybe #18413, possibly others), which I don’t want to do at this point. I’d be happy to remove the fuzz job on this branch, and just have them running on master. If anyone agrees I’ll drop 9a8fb4cf4ba472a5c3e1b9b71d31673f881a4896 off here.

laanwj commented at 7:23 am on May 15, 2020: member

ACK on simply disabling fuzzers that don’t pass for now when they fail for a known reason (and perhaps fix them in later 0.20.x).

test: disable script fuzz tests

Given that #18413 has not been backported.

245c862cfd

fanquake force-pushed on May 15, 2020

promag commented at 9:20 am on May 15, 2020: member

Tested ACK 245c862cfd4883ea91b53d766abb00a9c3c1ea5c coin control with multiple wallets.

laanwj commented at 10:36 am on May 15, 2020: member

ACK 245c862cfd4883ea91b53d766abb00a9c3c1ea5c

MarcoFalke commented at 11:54 am on May 15, 2020: member

ACK 245c862cfd solved the conflicts myself as a sanity check. Did not re-review 🍷

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 245c862cfd solved the conflicts myself as a sanity check. Did not re-review 🍷
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg6kAv+LDQjoobwSPeclwoi0n8WEWBicnOXv7gq2Y9Iyq/bMkCuhHAzUev8NgNJ
 8KsTf/TH40c526SEwA8bQiVWYgCvdJPgvWo7IQumU4eZOZjNoGt/CvUdUgACuFYSP
 9FsrG4fsbA93BF/P/d+c2DmcHoDR2/ZSuJgrJaE1yRedEUEgO5haMjioxz6y6Oais
10/PXv37CYeV6TFF6hEf+l+7IcAKN1eWeMAGE0g5MhOhhPihSZPU6KNUUUkt0F/IK7
11tZuT2w3uk55QV7+zfLbZhOXgDf1gh9I4WaUpQwfg0JPECxmvO00aIGx9ytPg/iKQ
12H9Thom7whQw124Y6+rmfni3cahmo1pwIXMetLpKUQYQNNJun9BAlqTN/FSPhl4e4
13TcFEU9h6JU9cpGII5eP+z/x3pKYF1tVI6FLSr9Ti0/ByXDtY/N+FMtzfpDShg3KH
14IcO+/6yJ4O/SH5EOBEmJtcPnoCmueKEiBPydjxrbXbtsVrNqyvzkkaE0N+IyFGG/
1599TG/H6V
16=sXf4
17-----END PGP SIGNATURE-----

Timestamp of file with hash 2bea0a093c8813ed3d15e1a688aded6992ef7074d826b0bc8832010d2b4955ef -

MarcoFalke merged this on May 15, 2020

MarcoFalke closed this on May 15, 2020

fanquake deleted the branch on May 15, 2020

DrahtBot locked this on Feb 15, 2022

[0.20] Final backports for rc2 #18973