We need to lock the CNode's avalanche state data, because it will be accessed from multiple threads when we start using the scheduler to read the delegation for pairing it with a proof.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 16019 Build 31934: Build Diff build-without-wallet · build-debug · build-diff · build-clang-tidy · lint-circular-dependencies · build-clang Build 31933: arc lint + arc unit
Event Timeline
| src/avalanche/processor.cpp | ||
|---|---|---|
| 518–519 ↗ | (On Diff #28934) | you might want to slap an AssertLockHeld(pfrom->cs_avalanche_state) at the start of this method. the thread safety annotations aren't 100% consistent in my experience. |
| src/net.h | ||
| 1014 ↗ | (On Diff #28934) | this or the other struct also named AvalancheState within CNodeState in net_processing.cpp could really use a better name, I bugged out for a moment reading the code when I encountered both. |
Comment Actions
add an AssertLockHeld.
Regarding the use of the AvalancheState for two different structs, I agree that it is confusing. This is best done in a separate diff.
Comment Actions
Because the pointer is set and never mutated, you actually don't need the mutex to access the pointed data - which are immutable. The scope of these lock is too large. buildRemoteSighash for instance, shouldn't need the lock.
| src/avalanche/processor.cpp | ||
|---|---|---|
| 547 ↗ | (On Diff #28944) | It seems to me that this function is not useful. It has just one, it is not using any data from the processor, it doesn't looks like it belongs here. The code structure is the main reason why your lock are larger scope than they strictly actually need to be. |
| src/net.h | ||
| 963 ↗ | (On Diff #28944) | See here for instance. |
| 1013 ↗ | (On Diff #28944) | Put the mutex near the data it is guarding. |
Comment Actions
I don't think we have any protection against a peer sending us a new AvaHello message with a different delegation, at the moment. It would update m_avalanche_state->delegation if that happened.
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.
Tail of the build log:
-- Installing: /results/artifacts/include/secp256k1_recovery.h -- Installing: /results/artifacts/include/secp256k1_schnorr.h [332/508] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/uint256.cpp.o [333/508] Building CXX object src/CMakeFiles/util.dir/util/message.cpp.o [334/508] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o [335/508] Building CXX object src/CMakeFiles/util.dir/util/settings.cpp.o [336/508] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o [337/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o [338/508] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o [339/508] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o [340/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o [341/508] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o [342/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o [343/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o [344/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o [345/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o [346/508] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o [347/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o [348/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o [349/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o [350/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o [351/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o [352/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o [353/508] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o [354/508] Linking CXX static library src/libutil.a [355/508] Linking CXX static library src/librpcclient.a [356/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o [357/508] Linking CXX static library src/libbitcoinconsensus.a [358/508] Linking CXX static library src/libscript.a [359/508] Linking CXX static library src/libcommon.a [360/508] Linking CXX shared library src/libbitcoinconsensus.so.0.23.8 [361/508] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so [362/508] Linking CXX executable src/bitcoin-cli [363/508] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o [364/508] Linking CXX executable src/bitcoin-tx [365/508] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o [366/508] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o [367/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o [368/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o [369/508] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o [370/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o [371/508] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o [372/508] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o [373/508] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o [374/508] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o [375/508] Linking CXX static library src/zmq/libzmq.a [376/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o [377/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o [378/508] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o [379/508] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o [380/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o [381/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o [382/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o [383/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o [384/508] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o [385/508] Linking CXX static library src/wallet/libwallet.a [386/508] Linking CXX static library src/wallet/libwallet-tool.a [387/508] Linking CXX executable src/bitcoin-wallet ninja: build stopped: cannot make progress due to previous errors. Build build-clang failed with exit code 1
Comment Actions
add the lock also when using the key in AVARESPONSE (fixes clang error)
Move some code in the AVAHELLO message handling to reduce the scope of the lock to only the pieces that require pubkey.
Comment Actions
The description fo this patch makes no sense. We aren't using the scheduler to read delegations. I do not know why this lock is needed. It seems to me that creating the avalanche state once the delegation has been verified should be enough.
Comment Actions
You are right about the description, sorry about that. It definitely needs to be updated.
Comment Actions
All accesses to that data are done in the same thread (ProcessMessages), except for an RPC call. So the lock is unnecessary. It is very unlikely that a addavalanchenode call happens exactly at the same time as the same node sending the data.