Page MenuHomePhabricator

Release cs_main during InvalidateBlock iterations
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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 edited the summary of this revision. (Show Details)
fpelliccioni edited reviewers, added: markblundeberg; removed: Fabien.
fpelliccioni edited the summary of this revision. (Show Details)

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]]
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

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

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.

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