p2pEnabled was maybe useless, g_connman is always initialized before RPC server is getting out of warmup. These checks against P2P state were introduced in https://github.com/bitcoin/bitcoin/pull/8085/commits/5b446dd5b11d4f403554bc2dd5a7a94c7d20422a.
Also when CWalletTx::RelayWalletTransaction is called g_connman is already set and the scheduler is stopped before g_connman is released. AFAICT this change is correct. I was thinking adding assert(g_connman) in interfaces::ChainImpl::relayTransaction but probably it’s unnecessary.
ACK 18d6a81505123e703be87aa31eeb7fb48b5d50bd.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
~0 on this
Needs review by @theuni, he probably knows the reasoning behind this. I think the intent was that it’s possible to run the node without P2P enabled at all, even though it’s currently not an option.
(or maybe the idea was to make various parts more loosely coupled to have more flexibility in the initialization sequence)
assert(g_connman) in interfaces::ChainImpl::relayTransaction
Yes, please do this. Otherwise, in the case of a bug in the initialization sequence that skips the P2P initialization it runs into a null pointer exception.
(or maybe the idea was to make various parts more loosely coupled to have more flexibility in the initialization sequence) @laanwj I don’t think just checking
p2pEnabled()is enough for that, it’s not thread safe and it doesn’t retain theCConnMan.
I would argue that removing global use in ChainImpl move forward getting the P2P self-contained, which is an ongoing project I think.
Yes of course @theuni should know the reason and if checks are still relevant.
Note : #15713 is removing relayTransaction on the p2p check, wallet should just submit txn to mempool
ACK 18d6a81505123e703be87aa31eeb7fb48b5d50bd
assert(g_connman) in interfaces::ChainImpl::relayTransaction
Seems reasonable. I’d add a comment saying something like:
g_connman is assigned before chain clients are started, and reset after chain clients are stopped. g_connman should never be null for calls over the Chain interface.
unsigned ACK 6e089572a79de38a08692b23b8a139171082139b (only looked at the diff on GitHub)
I think it makes sense to remove obviously dead code. And if it wasn’t dead code, it would be racy and result in either an assert failure or a segmentation fault. If such a check is ever needed in the future, it should be added at that time.
Do you mind adding a comment to the assert(g_connman) in BroadcastTransaction()? Something like:
“BroadcastTransaction can be called by the RPC server or wallet. g_connman is assigned before chain clients and the RPC server are started, and reset after chain clients adn the RPC are stopped are stopped. g_connman should never be null here.”
RPC server starts in warmup mode, it can't
process yet calls, then follows connection manager
initialization and finally RPC server get out of
warmup mode. RPC calls shouldn't be able to get
P2P disabled errors because once we initialize
g_connman it's not unset until shutdown, after
RPC server has been stopped.
Sorry for the delay. Post-merge concept ACK.
Chain was the wrong place for this anyway.