Page MenuHomePhabricator

[backport#19178] Make mininode_lock non-reentrant
ClosedPublic

Authored by PiRK on Tue, Apr 20, 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 requested review of this revision.Tue, Apr 20, 14:09

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

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

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.Tue, Apr 20, 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.Tue, Apr 20, 15:42

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.

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.

Why are the changes to P2PTxInvStore ignored?

deadalnix requested changes to this revision.Wed, Apr 21, 18:51
This revision now requires changes to proceed.Wed, Apr 21, 18:51

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.Thu, Apr 22, 08:41
PiRK edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Thu, Apr 22, 17:38