Page MenuHomePhabricator
Differential D4802

Release cs_main during InvalidateBlock iterations
ClosedPublic
Actions

Authored by fpelliccioni on Dec 26 2019, 18:52.

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
  1. 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
  1. Verify that the compiler has not emitted a thread-safety warning.
  2. Run the node: ./src/bitcoind -regtest
  3. 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 8689
Build 15364: Default Diff Build & Tests
Build 15363: 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
teamcity added a comment.Dec 26 2019, 18:52

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)
teamcity added a comment.Dec 26 2019, 19:07
Build Bitcoin-ABC / Diffs / Diff Testing failed.

Snippet of first build failure:
lines=16,COUNTEREXAMPLE```[19:07:20] : [Step 1/1] [0m [0;34mrpc_createmultisig.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_decodescript.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_deprecated.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_estimatefee.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_fundrawtransaction.py | ✓ Passed | 32 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_getblockstats.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_getchaintips.py | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_help.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_invalidateblock.py | ✓ Passed | 6 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_named_arguments.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_net.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_preciousblock.py | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_psbt.py | ✓ Passed | 7 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_rawtransaction.py | ✓ Passed | 22 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_scantxoutset.py | ✓ Passed | 4 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_signmessage.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_signrawtransaction.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_txoutproof.py | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_uptime.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_users.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mrpc_zmq.py | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_abandonconflict.py | ✓ Passed | 7 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_basic.py | ✓ Passed | 26 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_disable.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_disableprivatekeys.py --usecli | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_dump.py | ✓ Passed | 3 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_encryption.py | ✓ Passed | 5 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_groups.py | ✓ Passed | 7 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_hd.py | ✓ Passed | 4 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_import_rescan.py | ✓ Passed | 4 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_importmulti.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_importprunedfunds.py | ✓ Passed | 0 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_keypool.py | ✓ Passed | 3 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_keypool_topup.py | ✓ Passed | 3 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_labels.py | ✓ Passed | 4 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_listreceivedby.py | ✓ Passed | 13 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_listsinceblock.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_listtransactions.py | ✓ Passed | 8 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_multiwallet.py | ✓ Passed | 6 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_multiwallet.py --usecli | ✓ Passed | 7 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_resendwallettransactions.py | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_clone.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_clone.py --mineblock | ✓ Passed | 1 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_txn_doublespend.py --mineblock | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;34mwallet_zapwallettxes.py | ✓ Passed | 2 s
[19:07:20] : [Step 1/1] [0m [0;31mfeature_notifications.py | ✖ Failed | 11 s
[19:07:20] : [Step 1/1] [0m [0;31m [1m
[19:07:20] : [Step 1/1] ALL | ✖ Failed | 413 s (accumulated)
[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/sanitizer_logs/*.log.* *
[19:07:20]W: [Step 1/1] ++ print_sanitizers_log
[19:07:20]W: [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[19:07:20]W: [Step 1/1] ++ echo '* Output of /tmp/sanitizer_logs/*.log.* *'
[19:07:20]W: [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[19:07:20]W: [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[19:07:20]W: [Step 1/1] Process exited with code 1
[19:07:21]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=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
Fabien requested changes to this revision.Jan 2 2020, 11:06
Fabien added a subscriber: Fabien.

Please investigate why the build failed

This revision now requires changes to proceed.Jan 2 2020, 11:06
fpelliccioni updated this revision to Diff 15112.Jan 2 2020, 20:41

rebasing

teamcity added a comment.Jan 2 2020, 20:41

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 2 2020, 20:54
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
deadalnix requested changes to this revision.Jan 3 2020, 06:02

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
fpelliccioni updated this revision to Diff 15135.Jan 3 2020, 16:21

fixes tests

teamcity added a comment.Jan 3 2020, 16:21

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 3 2020, 16:25

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
fpelliccioni updated this revision to Diff 15192.Jan 7 2020, 15:32

fixes suggested

teamcity added a comment.Jan 7 2020, 15:33

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 7 2020, 15:40

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.

fpelliccioni updated this revision to Diff 15242.Jan 8 2020, 18:46

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

teamcity added a comment.Jan 8 2020, 18:46

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 8 2020, 18:53

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.

This revision is now accepted and ready to land.Jan 10 2020, 14:44
fpelliccioni added a child revision: D4942: Call FinalizeBlockAndInvalidate without cs_main held.Jan 14 2020, 15:52
fpelliccioni updated this revision to Diff 15451.Jan 14 2020, 15:56

rebased from master

teamcity added a comment.Jan 14 2020, 15:56

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 14 2020, 16:01

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8932: Diff 15451.Jan 14 2020, 16:01
Closed by commit rABC3fb45c049a4c: Release cs_main during InvalidateBlock iterations (authored by Pieter Wuille <pieter.wuille@gmail.com>, committed by fpelliccioni). · Explain WhyJan 14 2020, 16:05
This revision was automatically updated to reflect the committed changes.
fpelliccioni added a commit: rABC3fb45c049a4c: Release cs_main during InvalidateBlock iterations.
markblundeberg mentioned this in D4929: Make last disconnected block BLOCK_FAILED_VALID, even when aborted.Jan 18 2020, 02:47
markblundeberg mentioned this in D5013: CheckBlockIndex before unlocking cs_main.Jan 18 2020, 03:06
fpelliccioni added a commit: rSTAGING3fb45c049a4c: Release cs_main during InvalidateBlock iterations.Apr 7 2020, 22:26

Revision Contents
Changeset List

PathSize
src/
2 lines
107 lines

Diff 15084

View Options

src/validation.h

Loading...
View Options

src/validation.cpp

Loading...