Details

Reviewers

deadalnix
markblundeberg
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING3fb45c049a4c: Release cs_main during InvalidateBlock iterations
rABC3fb45c049a4c: Release cs_main during InvalidateBlock iterations

Summary

This PR makes a number of improvements to the InvalidateBlock (invalidateblock RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition:

They're made safely interruptible (bitcoind can be shutdown, and no progress in either will be lost, though if incomplete, invalidateblock won't continue after restart and will need to be called again)
The validation queue is prevented from overflowing (meaning invalidateblock on a very old block will not drive bitcoind OOM) (see #14289).
invalidateblock won't bother to move transactions back into the mempool after 10 blocks (optimization).

Partial Backport of Bitcoin Core PR15402 (commit 241b2c7):
https://github.com/bitcoin/bitcoin/pull/15402/commits/9bb32eb571a846b66ed3bac493f55cee11a3a1b9

Release cs_main during InvalidateBlock iterations

Depends on D4758

Test Plan

Build with Clang in Debug mode:

CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check-all

Verify that the compiler has not emitted a thread-safety warning.
Run the node: ./src/bitcoind -regtest
Verify that text similar to "Assertion failed: lock ... not held ..." is not printed on stderr.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

feature-backport-3db0cc394-9bb32eb

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8932
Build 15838: Default Diff Build & Tests
Build 15837: arc lint + arc unit

Event Timeline

fpelliccioni created this revision.Dec 26 2019, 18:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 26 2019, 18:52

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Release cs_main during InvalidateBlock iterations.Dec 26 2019, 18:54

fpelliccioni edited the summary of this revision. (Show Details)

fpelliccioni edited the test plan for this revision. (Show Details)

fpelliccioni added a parent revision: D4758: Call InvalidateBlock without cs_main held.

fpelliccioni edited the summary of this revision. (Show Details)

fpelliccioni edited reviewers, added: markblundeberg; removed: Fabien.

fpelliccioni edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Snippet of first build failure:
lines=16,COUNTEREXAMPLE```[19:07:20] : [Step 1/1] [19:07:20] : [Step 1/1] [0m [0;34mrpc_decodescript.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_deprecated.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_estimatefee.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_fundrawtransaction.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_getblockstats.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_getchaintips.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_help.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_invalidateblock.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_named_arguments.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_net.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_preciousblock.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_psbt.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_rawtransaction.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_scantxoutset.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_signmessage.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_signrawtransaction.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_txoutproof.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_uptime.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_users.py [19:07:20] : [Step 1/1] [0m [0;34mrpc_zmq.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_abandonconflict.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_basic.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_disable.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_dump.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_encryption.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_groups.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_hd.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_import_rescan.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_importmulti.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_importprunedfunds.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_keypool.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_keypool_topup.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_labels.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_listreceivedby.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_listsinceblock.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_listtransactions.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_multiwallet.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_multiwallet.py --usecli [19:07:20] : [Step 1/1] [0m [0;34mwallet_resendwallett [19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_clone.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_clone.py --mineblock [19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py [19:07:20] : [Step 1/1] [0m [0;34mwallet_zapwallettxes.py [19:07:20] : [Step 1/1] [0m [0;31mfeature_notifications.py [19:07:20] : [Step 1/1] [0m [0;31m [1m
[19:07:20] : [Step 1/1] ALL [19:07:20] : [Step 1/1] [0m [0mRuntime: 236 s
[19:07:20] : [Step 1/1]
[19:07:20] : [Step 1/1] * Output of /tmp/saniti [19:07:20]W: [Step 1/1] ++ print_sanitizers_log
[19:07:20]W: [Step 1/1] ++ for log in "${SAN_LOG_D [19:07:20]W: [Step 1/1] ++ echo '* [19:07:20]W: [Step 1/1] ++ cat '/tmp/sanitizer_log [19:07:20]W: [Step 1/1] cat: '/tmp/sanitizer_logs/ [19:07:20]W: [Step 1/1] Process exited with code [19:07:21]E: [Step 1/1] Process exited with code [0m [0;34mrpc_createmultisig.py | ✓ Passed | 2 s
| ✓ Passed | 0 s
| ✓ Passed | 0 s
| ✓ Passed | 0 s
| ✓ Passed | 32 s
| ✓ Passed | 0 s
| ✓ Passed | 1 s
| ✓ Passed | 0 s
| ✓ Passed | 6 s
| ✓ Passed | 0 s
| ✓ Passed | 0 s
| ✓ Passed | 1 s
| ✓ Passed | 7 s
| ✓ Passed | 22 s
| ✓ Passed | 4 s
| ✓ Passed | 0 s
| ✓ Passed | 0 s
| ✓ Passed | 1 s
| ✓ Passed | 0 s
| ✓ Passed | 2 s
| ✓ Passed | 1 s
| ✓ Passed | 7 s
| ✓ Passed | 26 s
| ✓ Passed | 0 s
| ✓ Passed | 0 s
--usecli | ✓ Passed | 0 s
| ✓ Passed | 3 s
| ✓ Passed | 5 s
| ✓ Passed | 7 s
| ✓ Passed | 4 s
| ✓ Passed | 4 s
| ✓ Passed | 2 s
| ✓ Passed | 0 s
| ✓ Passed | 3 s
| ✓ Passed | 3 s
| ✓ Passed | 4 s
| ✓ Passed | 13 s
| ✓ Passed | 2 s
| ✓ Passed | 8 s
| ✓ Passed | 6 s
| ✓ Passed | 7 s
ransactions.py | ✓ Passed | 1 s
| ✓ Passed | 2 s
| ✓ Passed | 1 s
| ✓ Passed | 2 s
--mineblock | ✓ Passed | 2 s
| ✓ Passed | 2 s
| ✖ Failed | 11 s
| ✖ Failed | 413 s (accumulated)
zer_logs/*.log.* *
IR}"/*.log.*
Output of /tmp/sanitizer_logs/*.log.* *'
s/*.log.*'
*.log.*': No such file or directory
1
1 (Step: Command Line)

Each failure log is accessible here:
[[https://build.bitcoinabc.org/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=24772&guest=1&_focus=2436 | bitcoin_abc_tests: feature_notifications.py]]

Harbormaster failed remote builds in B8689: Diff 15084!Dec 26 2019, 19:07

fpelliccioni added a child revision: D4803: Prevent callback overruns in InvalidateBlock and RewindBlockIndex.Dec 26 2019, 19:18

Please investigate why the build failed

This revision now requires changes to proceed.Jan 2 2020, 11:06

rebasing

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Snippet of first build failure:
lines=16,COUNTEREXAMPLE```[20:54:29] : [Step 1/1] [0m [0;34mrpc_createmultisig.py | ✓ Passed | 3 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_decodescript.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_deprecated.py | ✓ Passed | 1 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_estimatefee.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_fundrawtransaction.py | ✓ Passed | 34 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_getblockstats.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_getchaintips.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_help.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_invalidateblock.py | ✓ Passed | 5 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_named_arguments.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_net.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_preciousblock.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_psbt.py | ✓ Passed | 8 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_rawtransaction.py | ✓ Passed | 30 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_scantxoutset.py | ✓ Passed | 4 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_signmessage.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_signrawtransaction.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_txoutproof.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_uptime.py | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_users.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mrpc_zmq.py | ✓ Passed | 1 s
[20:54:29]W: [Step 1/1] ++ print_sanitizers_log
[20:54:29] : [Step 1/1] [0m [0;34mwallet_abandonconflict.py | ✓ Passed | 3 s
[20:54:29]W: [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[20:54:29] : [Step 1/1] [0m [0;34mwallet_basic.py | ✓ Passed | 31 s
[20:54:29]W: [Step 1/1] ++ echo '* Output of /tmp/sanitizer_logs/*.log.* *'
[20:54:29] : [Step 1/1] [0m [0;34mwallet_disable.py | ✓ Passed | 0 s
[20:54:29]W: [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[20:54:29] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py | ✓ Passed | 0 s
[20:54:29]W: [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[20:54:29] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py --usecli | ✓ Passed | 0 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_dump.py | ✓ Passed | 3 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_encryption.py | ✓ Passed | 5 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_groups.py | ✓ Passed | 5 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_hd.py | ✓ Passed | 5 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_import_rescan.py | ✓ Passed | 4 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_importmulti.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_importprunedfunds.py | ✓ Passed | 1 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_keypool.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_keypool_topup.py | ✓ Passed | 3 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_labels.py | ✓ Passed | 4 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_listreceivedby.py | ✓ Passed | 12 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_listsinceblock.py | ✓ Passed | 3 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_listtransactions.py | ✓ Passed | 8 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_multiwallet.py | ✓ Passed | 7 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_multiwallet.py --usecli | ✓ Passed | 8 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_resendwallettransactions.py | ✓ Passed | 1 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_txn_clone.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_txn_clone.py --mineblock | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py --mineblock | ✓ Passed | 1 s
[20:54:29] : [Step 1/1] [0m [0;34mwallet_zapwallettxes.py | ✓ Passed | 2 s
[20:54:29] : [Step 1/1] [0m [0;31mfeature_notifications.py | ✖ Failed | 11 s
[20:54:29] : [Step 1/1] [0m [0;31m [1m
[20:54:29] : [Step 1/1] ALL | ✖ Failed | 433 s (accumulated)
[20:54:29] : [Step 1/1] [0m [0mRuntime: 247 s
[20:54:29] : [Step 1/1]
[20:54:29] : [Step 1/1] * Output of /tmp/sanitizer_logs/*.log.* *
[20:54:29]W: [Step 1/1] Process exited with code 1
[20:54:29]E: [Step 1/1] Process exited with code 1 (Step: Command Line)

Each failure log is accessible here:
[[https://build.bitcoinabc.org/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=24883&guest=1&_focus=1987 | bitcoin_abc_tests: feature_notifications.py]]

Harbormaster failed remote builds in B8709: Diff 15112!Jan 2 2020, 20:54

Back on your plate.

This revision now requires changes to proceed.Jan 3 2020, 06:02

fpelliccioni planned changes to this revision.Jan 3 2020, 12:54

fixes tests

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8724: Diff 15135.Jan 3 2020, 16:25

deadalnix requested changes to this revision.Jan 5 2020, 10:27

deadalnix added inline comments.

src/validation.cpp
3034 ↗	(On Diff #15135)	braces
3038 ↗	(On Diff #15135)	braces
3055 ↗	(On Diff #15135)	braces
3074 ↗	(On Diff #15135)	This is not an appropriate name.
3089 ↗	(On Diff #15135)	This is a clue that the name is no good, because to_mark_failed is not always marked failed but sometime parked.
3090 ↗	(On Diff #15135)	You are missing the change to setBlockIndexCandidates

This revision now requires changes to proceed.Jan 5 2020, 10:27

fixes suggested

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8761: Diff 15192.Jan 7 2020, 15:40

deadalnix requested changes to this revision.Jan 7 2020, 15:50

deadalnix added inline comments.

src/validation.cpp
3099 ↗	(On Diff #15192)	So that doesn't match the source material. I'm not convinced that these calls are necessary at all, but are they?

This revision now requires changes to proceed.Jan 7 2020, 15:50

fpelliccioni added inline comments.Jan 7 2020, 16:01

src/validation.cpp
3099 ↗	(On Diff #15192)	What do you mean by "source material"? The code in Core commit or our current code? I think this line is not necessary, but I think you ask me to add it in a previous comment.

deadalnix added inline comments.Jan 8 2020, 17:38

src/validation.cpp
3099 ↗	(On Diff #15192)	I wrote that it was missing compared to the PR. Which means it either need to be added, or at least we need to ensure that differences in our approach make it obsolete. In any case, the orginial PR adds the removal of `to_mark_failed_or_parked->pprev` so it doesn't match what the original code added. But as you also noticed `to_mark_failed_or_parked` was not present in our codebase - but it was in core's. So it is worth looking at why this difference exists and if it is a bug on our side, a bug on core's, or if other differences int he codebase justify that difference.

Remove code that removes from setBlockIndexCandidates as FindMostWorkChain() is in charge of it.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8800: Diff 15242.Jan 8 2020, 18:53

deadalnix accepted this revision.Jan 9 2020, 13:06

fpelliccioni edited the test plan for this revision. (Show Details)Jan 10 2020, 13:47

Fabien accepted this revision.Jan 10 2020, 14:44

Fabien added inline comments.

src/validation.cpp
3048 ↗	(On Diff #15242)	I also like the newlines, but keeping the comments with it's code block is better imo. Not a strong case.
3087 ↗	(On Diff #15242)	This comment is at the wrong place.
3095 ↗	(On Diff #15242)	Should be above this.