Remove p2pEnabled from Chain interface #16503

pull ariard wants to merge 1 commits into bitcoin:master from ariard:2019-07-remove-p2p-chain changing 4 files +3 −12

ariard commented at 11:08 PM on July 30, 2019: member

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. @mzumsande comment in #15713 let me thought that 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.
promag commented at 11:46 PM on July 30, 2019: member

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.
DrahtBot added the label RPC/REST/ZMQ on Jul 30, 2019
DrahtBot added the label Wallet on Jul 30, 2019
DrahtBot commented at 2:41 AM on July 31, 2019: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
laanwj requested review from theuni on Jul 31, 2019
laanwj commented at 6:14 AM on July 31, 2019: member

~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.
fanquake added the label Waiting for author on Jul 31, 2019
promag commented at 7:43 AM on July 31, 2019: member

(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 the CConnMan.
ariard commented at 2:29 PM on July 31, 2019: member

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
MarcoFalke commented at 3:19 PM on July 31, 2019: member

Concept ACK per @ariard and @promag . I prefer the way #15713 does it by returning TransactionError::P2P_DISABLED when connman is not there.
jnewbery commented at 3:39 PM on August 1, 2019: member

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.
DrahtBot added the label Needs rebase on Aug 2, 2019
MarcoFalke removed the label Waiting for author on Aug 7, 2019
MarcoFalke commented at 8:32 PM on August 7, 2019: member

@ariard Are you still working on this?
ariard commented at 8:53 PM on August 7, 2019: member

Yes, still waiting Concept ACK by @theuni to go further
jnewbery commented at 10:00 PM on August 7, 2019: member

No need to allow this to be blocked by one person. I suggest you keep this rebased so it's ready to merge.
ariard force-pushed on Aug 8, 2019
ariard commented at 2:00 AM on August 8, 2019: member

Rebased at 6e08957. RelayWalletTransaction P2P check has been removed due to #15713 merge, which also added a g_connman assertion in BroadcastTransaction.
DrahtBot removed the label Needs rebase on Aug 8, 2019
MarcoFalke commented at 6:32 PM on August 8, 2019: member

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.
jnewbery commented at 6:44 PM on August 8, 2019: member

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

Remove p2pEnabled from Chain interface

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.

b7b9f6e4ce

ariard force-pushed on Aug 9, 2019
ariard commented at 3:00 AM on August 9, 2019: member

Comment added at b7b9f6e
promag commented at 7:40 AM on August 9, 2019: member

ACK b7b9f6e4cee262004643e2fe03d56cb47fdbf5c2
jnewbery commented at 1:34 PM on August 9, 2019: member

ACK b7b9f6e4cee262004643e2fe03d56cb47fdbf5c2
MarcoFalke referenced this in commit 9ab9d63569 on Aug 9, 2019
MarcoFalke merged this on Aug 9, 2019
MarcoFalke closed this on Aug 9, 2019
theuni commented at 1:54 PM on August 10, 2019: member

Sorry for the delay. Post-merge concept ACK.

Chain was the wrong place for this anyway.
deadalnix referenced this in commit a4830c0892 on Sep 5, 2020
Munkybooty referenced this in commit 5d4969b10b on Nov 25, 2021
Munkybooty referenced this in commit 420ddf58ff on Nov 25, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Review Requested
theuni

Labels

Wallet RPC/REST/ZMQ

Linked (view graph)

#8085 p2p: Begin encapsulation #15713 refactor: Replace chain relayTransactions/submitMemoryPool by higher method

Remove p2pEnabled from Chain interface #16503

Conflicts