Page MenuHomePhabricator
Differential D9430

[backport#19178] Make mininode_lock non-reentrant
ClosedPublic
Actions

Authored by PiRK on Apr 20 2021, 14:09.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCe7039654cdb3: [backport#19178] Make mininode_lock non-reentrant
Summary

https://github.com/bitcoin/bitcoin/pull/19178/commits/edae6075aa3b1169c84b65e76fd48d68242a294e
[tests] Only acquire lock once in p2p_compactblocks.py

https://github.com/bitcoin/bitcoin/pull/19178/commits/9d80762fa0931fe553fad241e95bcc1515ef0e95
[tests] Don't acquire mininode_lock twice in wait_for_broadcast()

https://github.com/bitcoin/bitcoin/pull/19178/commits/c67c1f2c032a8efa141d776a7e5be58f052159ea
[tests] Make mininode_lock non-reentrant

Commit c67c1f2c032a8efa141d776a7e5be58f052159ea removing a double call to the parent method in P2PTxInvStore.on_inv is not relevant for Bitcoin ABC, as it fixes a bug that was never introduced in our codebase. We skipped that commit while backporting core#18807.

To make it work, the following additional change is needed for Bitcoin ABC:
Don't acquire mininode_lock twice in abc_p2p_avalanche.py. on_avahello, on_avapoll and on_avaresponse are called indirectly via mininode.P2PInterface.on_message, whit the lock already acquired.

This is a backport of core#19178

Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Apr 20 2021, 14:09
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 20 2021, 14:09
PiRK requested review of this revision.Apr 20 2021, 14:09
teamcity edited the summary of this revision. (Show Details)Apr 20 2021, 14:09
Harbormaster completed remote builds in B15541: Diff 28218.Apr 20 2021, 14:34
teamcity added a comment.Apr 20 2021, 14:34
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

feature_asmap.py started
wallet_avoidreuse.py passed, Duration: 14 s
feature_cltv.py started
abc-transaction-ordering.py passed, Duration: 4 s
feature_dersig.py started
wallet_import_rescan.py passed, Duration: 10 s
feature_settings.py started
feature_asmap.py passed, Duration: 4 s
feature_uacomment.py started
feature_settings.py passed, Duration: 3 s
interface_bitcoin_cli.py started
feature_uacomment.py passed, Duration: 3 s
mempool_reorg.py started
interface_bitcoin_cli.py passed, Duration: 3 s
p2p_unrequested_blocks.py started
feature_cltv.py passed, Duration: 9 s
rpc_createmultisig.py started
feature_dersig.py passed, Duration: 8 s
rpc_scantxoutset.py started
mempool_reorg.py passed, Duration: 4 s
wallet_createwallet.py --usecli started
p2p_unrequested_blocks.py passed, Duration: 4 s
wallet_importmulti.py started
rpc_createmultisig.py passed, Duration: 4 s
wallet_keypool.py started
wallet_createwallet.py --usecli passed, Duration: 3 s
wallet_reorgsrestore.py started
wallet_keypool.py passed, Duration: 3 s
wallet_txn_clone.py --mineblock started
rpc_scantxoutset.py passed, Duration: 7 s
wallet_txn_doublespend.py --mineblock started
wallet_importmulti.py passed, Duration: 5 s
abc-cmdline.py started
wallet_reorgsrestore.py passed, Duration: 4 s
abc-finalize-block.py started
abc-cmdline.py passed, Duration: 2 s
abc-invalid-chains.py started
wallet_txn_clone.py --mineblock passed, Duration: 4 s
abc-mempool-coherence-on-activations.py started
abc-finalize-block.py passed, Duration: 2 s
abc-minimaldata.py started
wallet_txn_doublespend.py --mineblock passed, Duration: 3 s
abc-replay-protection.py started
abc-invalid-chains.py passed, Duration: 2 s
abc-schnorrmultisig.py started
abc-minimaldata.py passed, Duration: 2 s
abc-segwit-recovery.py started
abc-mempool-coherence-on-activations.py passed, Duration: 2 s
feature_includeconf.py started
abc-replay-protection.py passed, Duration: 2 s
feature_reindex.py started
abc-schnorrmultisig.py passed, Duration: 2 s
feature_shutdown.py started
abc-segwit-recovery.py passed, Duration: 2 s
mempool_accept.py started
feature_includeconf.py passed, Duration: 2 s
mining_basic.py started
feature_shutdown.py passed, Duration: 1 s
p2p_disconnect_ban.py started
Build build-debug timed out after 1200.0s
teamcity added a comment.Apr 20 2021, 14:34
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

rpc_scantxoutset.py started
rpc_scantxoutset.py skipped
wallet_createwallet.py --usecli started
wallet_createwallet.py --usecli skipped
wallet_importmulti.py started
wallet_importmulti.py skipped
wallet_keypool.py started
wallet_keypool.py skipped
wallet_reorgsrestore.py started
wallet_reorgsrestore.py skipped
wallet_txn_clone.py --mineblock started
wallet_txn_clone.py --mineblock skipped
wallet_txn_doublespend.py --mineblock started
wallet_txn_doublespend.py --mineblock skipped
abc-cmdline.py started
interface_bitcoin_cli.py passed, Duration: 2 s
abc-finalize-block.py started
feature_uacomment.py passed, Duration: 3 s
abc-invalid-chains.py started
abc-cmdline.py passed, Duration: 2 s
abc-mempool-coherence-on-activations.py started
abc-invalid-chains.py passed, Duration: 2 s
abc-minimaldata.py started
abc-finalize-block.py passed, Duration: 2 s
abc-replay-protection.py started
p2p_unrequested_blocks.py passed, Duration: 3 s
abc-schnorrmultisig.py started
abc-minimaldata.py passed, Duration: 2 s
abc-segwit-recovery.py started
abc-mempool-coherence-on-activations.py passed, Duration: 2 s
feature_includeconf.py started
abc-replay-protection.py passed, Duration: 2 s
feature_reindex.py started
abc-schnorrmultisig.py passed, Duration: 3 s
feature_shutdown.py started
abc-segwit-recovery.py passed, Duration: 2 s
mempool_accept.py started
mempool_accept.py skipped
mining_basic.py started
feature_shutdown.py passed, Duration: 1 s
p2p_disconnect_ban.py started
feature_includeconf.py passed, Duration: 2 s
p2p_dos_header_tree.py started
feature_reindex.py passed, Duration: 3 s
p2p_filter.py started
p2p_filter.py skipped
p2p_invalid_locator.py started
mining_basic.py passed, Duration: 2 s
rpc_bind.py --ipv4 started
p2p_dos_header_tree.py passed, Duration: 2 s
rpc_bind.py --ipv6 started
p2p_disconnect_ban.py passed, Duration: 2 s
rpc_setban.py started
p2p_invalid_locator.py passed, Duration: 2 s
rpc_txoutproof.py started
rpc_txoutproof.py skipped
wallet_createwallet.py started
wallet_createwallet.py skipped
wallet_importprunedfunds.py started
Build build-without-wallet timed out after 1200.0s
teamcity added a comment.Apr 20 2021, 14:35
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

feature_asmap.py started
wallet_zapwallettxes.py passed, Duration: 3 s
feature_cltv.py started
abc-schnorr.py passed, Duration: 2 s
feature_dersig.py started
abc-transaction-ordering.py passed, Duration: 3 s
feature_settings.py started
feature_dersig.py passed, Duration: 3 s
feature_uacomment.py started
feature_cltv.py passed, Duration: 3 s
interface_bitcoin_cli.py started
feature_asmap.py passed, Duration: 3 s
mempool_reorg.py started
feature_settings.py passed, Duration: 3 s
p2p_unrequested_blocks.py started
feature_uacomment.py passed, Duration: 3 s
rpc_createmultisig.py started
interface_bitcoin_cli.py passed, Duration: 3 s
rpc_scantxoutset.py started
mempool_reorg.py passed, Duration: 3 s
wallet_createwallet.py --usecli started
wallet_createwallet.py --usecli passed, Duration: 3 s
wallet_importmulti.py started
rpc_createmultisig.py passed, Duration: 3 s
wallet_keypool.py started
p2p_unrequested_blocks.py passed, Duration: 3 s
wallet_reorgsrestore.py started
rpc_scantxoutset.py passed, Duration: 3 s
wallet_txn_clone.py --mineblock started
wallet_importmulti.py passed, Duration: 3 s
wallet_txn_doublespend.py --mineblock started
wallet_keypool.py passed, Duration: 3 s
abc-cmdline.py started
wallet_reorgsrestore.py passed, Duration: 3 s
abc-finalize-block.py started
wallet_txn_clone.py --mineblock passed, Duration: 3 s
abc-invalid-chains.py started
abc-cmdline.py passed, Duration: 2 s
abc-mempool-coherence-on-activations.py started
abc-invalid-chains.py passed, Duration: 2 s
abc-minimaldata.py started
abc-finalize-block.py passed, Duration: 2 s
abc-replay-protection.py started
wallet_txn_doublespend.py --mineblock passed, Duration: 4 s
abc-schnorrmultisig.py started
abc-minimaldata.py passed, Duration: 2 s
abc-segwit-recovery.py started
abc-mempool-coherence-on-activations.py passed, Duration: 2 s
feature_includeconf.py started
abc-replay-protection.py passed, Duration: 2 s
feature_reindex.py started
abc-schnorrmultisig.py passed, Duration: 2 s
feature_shutdown.py started
abc-segwit-recovery.py passed, Duration: 2 s
mempool_accept.py started
feature_includeconf.py passed, Duration: 2 s
mining_basic.py started
feature_reindex.py passed, Duration: 2 s
p2p_disconnect_ban.py started
Build build-diff timed out after 1200.0s
deadalnix requested changes to this revision.Apr 20 2021, 15:42
deadalnix added a subscriber: deadalnix.

This is breaking the test suite, back to your queue. Please make sure you run it, this is happening way too often.

This revision now requires changes to proceed.Apr 20 2021, 15:42
PiRK added a comment.Apr 20 2021, 16:28
In D9430#214040, @deadalnix wrote:

This is breaking the test suite, back to your queue. Please make sure you run it, this is happening way too often.

In this case, the tests work on my machine. Not sure why these 3 particular builds fail. I will investigate.

PiRK updated this revision to Diff 28241.Apr 21 2021, 07:59

Don't acquire mininode_lock twice in abc_p2p_avalanche.py.
on_avahello, on_avapoll and on_avaresponse are called indirectly via mininode.P2PInterface.on_message, which does the call with the lock already acquired.

PiRK edited the summary of this revision. (Show Details)Apr 21 2021, 08:01
Harbormaster completed remote builds in B15553: Diff 28241.Apr 21 2021, 08:18
deadalnix added a comment.Apr 21 2021, 18:51

Why are the changes to P2PTxInvStore ignored?

deadalnix requested changes to this revision.Apr 21 2021, 18:51
This revision now requires changes to proceed.Apr 21 2021, 18:51
PiRK added a comment.EditedApr 22 2021, 08:37

When I backported core#18807, I did not pick the commit introducing this mistake (https://github.com/bitcoin/bitcoin/pull/18807/commits/00d44a534b4e5ae249b8011360c6b0f7dc731581), so the change to P2PTxInvStore is not relevant to our codebase.

I will update the revision summary to mention this.

PiRK requested review of this revision.Apr 22 2021, 08:41
PiRK edited the summary of this revision. (Show Details)
deadalnix accepted this revision.Apr 22 2021, 17:38
This revision is now accepted and ready to land.Apr 22 2021, 17:38
Closed by commit rABCe7039654cdb3: [backport#19178] Make mininode_lock non-reentrant (authored by John Newbery <john@johnnewbery.com>, committed by PiRK). · Explain WhyApr 23 2021, 08:53
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCe7039654cdb3: [backport#19178] Make mininode_lock non-reentrant.

Revision Contents
Changeset List

PathSize
test/
functional/
11 lines
7 lines
test_framework/
4 lines

Diff 28257

View Options

test/functional/abc_p2p_avalanche.py

Loading...
View Options

test/functional/p2p_compactblocks.py

Loading...
View Options

test/functional/test_framework/mininode.py

Loading...