Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[0.20] Final backports for rc2 #18973

Merged
merged 15 commits into from May 15, 2020
Merged

[0.20] Final backports for rc2 #18973

merged 15 commits into from May 15, 2020

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented May 14, 2020 •
edited

Backports the following PRs to the 0.20 branch:

@fanquake fanquake added this to the 0.20.0 milestone May 14, 2020
@luke-jr
Copy link
Member

luke-jr commented May 14, 2020

#18902 is also needed (fixes a regression)

@maflcko
Copy link
Member

maflcko commented May 14, 2020

#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

@fanquake fanquake mentioned this pull request May 14, 2020
Merged
@laanwj
Copy link
Member

laanwj commented May 14, 2020 •
edited

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).

@maflcko
Copy link
Member

maflcko commented May 14, 2020

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

@maflcko
Copy link
Member

maflcko commented May 14, 2020

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

@maflcko maflcko closed this May 14, 2020
@maflcko maflcko reopened this May 14, 2020
@maflcko
Copy link
Member

maflcko commented May 14, 2020

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

@laanwj
Copy link
Member

laanwj commented May 14, 2020

I think you need the following:

    squashme: remove const to work around compiler error on xenial
    
        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;

diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp
index d2fc20e625afaff0f262cf6ee0437cdaccb2791a..ceba689e52970ee5fff32682742d77bc2df48c63 100644
--- a/src/test/validationinterface_tests.cpp
+++ b/src/test/validationinterface_tests.cpp
@@ -23,7 +23,7 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
     // Start thread to generate notifications
     std::thread gen{[&] {
         const CBlock block_dummy;
-        const BlockValidationState state_dummy;
+        BlockValidationState state_dummy;
         while (generate) {
             GetMainSignals().BlockChecked(block_dummy, state_dummy);
         }

@maflcko
Copy link
Member

maflcko commented May 14, 2020

@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
Copy link
Member

laanwj commented May 14, 2020

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

@maflcko
Copy link
Member

maflcko commented May 14, 2020

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

@maflcko maflcko closed this May 14, 2020
@maflcko maflcko reopened this May 14, 2020
promag and others added 13 commits May 15, 2020 07:42
@promag @fanquake
gui: Fix itemWalletAddress leak when not tree mode
315ae14 
Github-Pull: bitcoin#18578
Rebased-From: e8123ea
@amitiuttarwar @fanquake
[net processing] ignore tx GETDATA from blocks-only peers
fb82173 
Co-Authored-By: John Newbery <john@johnnewbery.com>

Github-Pull: bitcoin#18808
Rebased-From: 047ceac
@amitiuttarwar @fanquake
[net processing] ignore unknown INV types in GETDATA messages
1e73d72 
Co-Authored-By: John Newbery <john@johnnewbery.com>

Github-Pull: bitcoin#18808
Rebased-From: e257cf7
@amitiuttarwar @fanquake
[test] test that an invalid GETDATA doesn't prevent processing of fut…
011532e 
…ure messages

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

Github-Pull: bitcoin#18808
Rebased-From: 2f03255
@jnewbery @fanquake
[docs] Improve commenting in ProcessGetData()
a3fe458 
Github-Pull: bitcoin#18808
Rebased-From: 9847e20
@promag @fanquake
rpc: Add mutex to guard deadlineTimers
ca4dac4 
Github-Pull: bitcoin#18814
Rebased-From: a2e6db5
@promag @fanquake
rpc: Relock wallet only if most recent callback
251e321 
Github-Pull: bitcoin#18814
Rebased-From: 9f59dde
@ryanofsky @fanquake
test: Add test for conflicted wallet tx notifications
ed0afe8 
Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

bitcoin#9240 - accidental break
bitcoin#9479 - bug report
bitcoin#9371 - fix
bitcoin#16624 - accidental break
bitcoin#18325 - bug report
bitcoin#18600 - potential fix

Github-Pull: bitcoin#18878
Rebased-From: f963a68
@promag @fanquake
gui: Fix manual coin control with multiple wallets loaded
ff4dc20 
Github-Pull: bitcoin#18894
Rebased-From: a8b5f1b
@fanquake
test: Add unregister_validation_interface_race test
37a6207 
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: bitcoin#18742
Rebased-From: fab6d06
@fanquake
miner: Avoid stack-use-after-return in validationinterface
cc7d344 
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: bitcoin#18742
Rebased-From: 7777f2a
@laanwj @fanquake
test: Remove const to work around compiler error on xenial
cf2a6e2 
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: bitcoin#18975
Rebased-From: 050e2ee
@jnewbery @fanquake
[net processing] Only send a getheaders for one block in an INV
6161c94 
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: bitcoin#18962
Rebased-From: 7467366
@fanquake
Copy link
Member Author

Rebased to include 7d87ba0 and aa7c685, and added #18962 and #18975.

@fanquake
Copy link
Member Author

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

I've now added this as well.

@fanquake
Copy link
Member Author

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 9a8fb4c off here.

@laanwj
Copy link
Member

laanwj commented May 15, 2020

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).

@fanquake
test: disable script fuzz tests
245c862 
Given that bitcoin#18413 has not been backported.
@promag
Copy link
Member

promag commented May 15, 2020

Tested ACK 245c862 coin control with multiple wallets.

@laanwj
Copy link
Member

laanwj commented May 15, 2020

ACK 245c862

@maflcko
Copy link
Member

maflcko commented May 15, 2020

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg6kAv+LDQjoobwSPeclwoi0n8WEWBicnOXv7gq2Y9Iyq/bMkCuhHAzUev8NgNJ
KsTf/TH40c526SEwA8bQiVWYgCvdJPgvWo7IQumU4eZOZjNoGt/CvUdUgACuFYSP
FsrG4fsbA93BF/P/d+c2DmcHoDR2/ZSuJgrJaE1yRedEUEgO5haMjioxz6y6Oais
/PXv37CYeV6TFF6hEf+l+7IcAKN1eWeMAGE0g5MhOhhPihSZPU6KNUUUkt0F/IK7
tZuT2w3uk55QV7+zfLbZhOXgDf1gh9I4WaUpQwfg0JPECxmvO00aIGx9ytPg/iKQ
H9Thom7whQw124Y6+rmfni3cahmo1pwIXMetLpKUQYQNNJun9BAlqTN/FSPhl4e4
TcFEU9h6JU9cpGII5eP+z/x3pKYF1tVI6FLSr9Ti0/ByXDtY/N+FMtzfpDShg3KH
IcO+/6yJ4O/SH5EOBEmJtcPnoCmueKEiBPydjxrbXbtsVrNqyvzkkaE0N+IyFGG/
99TG/H6V
=sXf4
-----END PGP SIGNATURE-----

Timestamp of file with hash 2bea0a093c8813ed3d15e1a688aded6992ef7074d826b0bc8832010d2b4955ef -

@maflcko maflcko merged commit 17bdf2a into bitcoin:0.20 May 15, 2020
@fanquake fanquake deleted the 0_20_0rc2_final_backports branch May 15, 2020 11:57
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Backport
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@fanquake @luke-jr @maflcko @laanwj @promag @amitiuttarwar @jnewbery @ryanofsky @practicalswift