Skip to content

Remove p2pEnabled from Chain interface #16503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Aug 9, 2019
Merged

Remove p2pEnabled from Chain interface #16503

maflcko merged 1 commit into from
Aug 9, 2019

Conversation

@ariard
Copy link

@ariard ariard commented Jul 30, 2019

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 5b446dd.

@promag
Copy link
Contributor

promag commented Jul 30, 2019

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
Copy link
Contributor

DrahtBot commented Jul 31, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@laanwj laanwj requested a review from theuni July 31, 2019 06:11
@laanwj
Copy link
Member

laanwj commented Jul 31, 2019 •
edited
Loading

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

@promag
Copy link
Contributor

promag commented Jul 31, 2019

(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
Copy link
Author

ariard commented Jul 31, 2019

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

@maflcko
Copy link
Member

maflcko commented Jul 31, 2019

Concept ACK per @ariard and @promag . I prefer the way #15713 does it by returning TransactionError::P2P_DISABLED when connman is not there.

@jnewbery
Copy link
Contributor

jnewbery commented Aug 1, 2019

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.

@maflcko maflcko mentioned this pull request Aug 2, 2019
Merged
@maflcko
Copy link
Member

maflcko commented Aug 7, 2019

@ariard Are you still working on this?

@ariard
Copy link
Author

ariard commented Aug 7, 2019 •
edited
Loading

Yes, still waiting Concept ACK by @theuni to go further

@jnewbery
Copy link
Contributor

jnewbery commented Aug 7, 2019

No need to allow this to be blocked by one person. I suggest you keep this rebased so it's ready to merge.

@ariard ariard force-pushed the 2019-07-remove-p2p-chain branch from 18d6a81 to 6e08957 Compare August 8, 2019 01:51
@ariard
Copy link
Author

ariard commented Aug 8, 2019

Rebased at 6e08957. RelayWalletTransaction P2P check has been removed due to #15713 merge, which also added a g_connman assertion in BroadcastTransaction.

@maflcko
Copy link
Member

maflcko commented Aug 8, 2019

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
Copy link
Contributor

jnewbery commented Aug 8, 2019

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
b7b9f6e 
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.
@ariard ariard force-pushed the 2019-07-remove-p2p-chain branch from 6e08957 to b7b9f6e Compare August 9, 2019 02:59
@ariard
Copy link
Author

ariard commented Aug 9, 2019

Comment added at b7b9f6e

@promag
Copy link
Contributor

promag commented Aug 9, 2019 •
edited
Loading

ACK b7b9f6e

1 similar comment
@jnewbery
Copy link
Contributor

jnewbery commented Aug 9, 2019

ACK b7b9f6e

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 9, 2019
Merge bitcoin#16503: Remove p2pEnabled from Chain interface
9ab9d63 
b7b9f6e Remove p2pEnabled from Chain interface (Antoine Riard)

Pull request description:

  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 bitcoin#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 bitcoin@5b446dd.

ACKs for top commit:
  promag:
    ACK b7b9f6e
  jnewbery:
    ACK b7b9f6e

Tree-SHA512: 4de2b9fc496bf8347ff5cc645848a5a44c8ca7596cd134f17f3088f5f8262d1d88b8e2a052df93e309ec9a81956a808df17a9eb9f10d4f4d693c95d607fe3561
@maflcko maflcko merged commit b7b9f6e into bitcoin:master Aug 9, 2019
@theuni
Copy link
Member

theuni commented Aug 10, 2019

Sorry for the delay. Post-merge concept ACK.

Chain was the wrong place for this anyway.

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 5, 2020
@majcosta
[backport#16503] Remove p2pEnabled from Chain interface
a4830c0 
Summary:
Remove p2pEnabled from Chain interface (Antoine Riard)

Pull request description:

  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 bitcoin/bitcoin@5b446dd.

---

Backport of Core [[bitcoin/bitcoin#16503 | PR16503]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7369
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@theuni theuni Awaiting requested review from theuni

Assignees

No one assigned

Labels

RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ariard @promag @DrahtBot @laanwj @maflcko @jnewbery @theuni @fanquake