Skip to content
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.

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

rpc, net: add erlay status in getpeerinfo #27797

Closed
wants to merge 2 commits into from

Conversation

brunoerg
Copy link
Contributor

Fixes #26602

Adds m_tx_reconciliation in Peer struct
to know whether the peer supports Erlay and
exposes it in getpeerinfo rpc.

Adds `m_tx_reconciliation` in `Peer` struct
to know whether the peer supports Erlay and
exposes it in `getpeerinfo` rpc.
@DrahtBot
Copy link
Contributor

DrahtBot commented May 31, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27742 ([NO MERGE] BIP331 Ancestor Package Relay by glozow)
  • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@brunoerg
Copy link
Contributor Author

cc: @Sjors

@Sjors
Copy link
Member

Sjors commented May 31, 2023

Concept ACK

@mzumsande
Copy link
Contributor

I'd definitely like this eventually, but I'm not sure how much sense it makes to have it before Erlay is functional (unfortunately, there hasn't been much progress lately in #26283, and even after this is merged Erlay still won't be functional yet).

@Sjors
Copy link
Member

Sjors commented May 31, 2023

@mzumsande given that we have -txreconciliation I think it's fine to add this. We can always drop it if Erlay is given up on. Making it easier to test, makes it more likely to ever get finished though.

@mzumsande
Copy link
Contributor

given that we have -txreconciliation I think it's fine to add this.

But -txreconciliation is off-by default and not exposed to users, this field exposes Erlay-related info unconditionally in a popular RPC, which could create confusion (why is this field there if I can't turn Erlay on yet?) and seems premature given that Erlay could still be years away (if it makes it at all, which I do hope!). So I'd prefer it if these commits were part of #21515 (which anyone wanting to actually test Erlay would need to run anyway) but not master for now.

Also, removing fields from RPCs tends to lead to annoying compatibility / depreciation discussions, so I'd prefer not adding fields in the first place until they are needed.

@brunoerg
Copy link
Contributor Author

I implemented this for testing purposes which have been very useful for me and could be for other reviewers, initially my idea is not open this, just did it after seeing #26602. However, I agree with @mzumsande about annoyability of removing fields from RPC, so I can leave this as "draft" for more discussions/not being merged and hopefully these commits can be picked in #21515 and then we can have it.

@fanquake
Copy link
Member

fanquake commented Jun 1, 2023

I think you could leave a comment in #21515, about cherry-picking this change into that PR, but I don't think there's a need to leave this PR open, if we aren't going to merge it. I agree with Martins comment above; it's premature to add this, and probably not something we should add to our RPC interface just as a convenience for developers, for testing a not-yet-implemented/experimental feature.

@fanquake fanquake closed this Jun 1, 2023
@naumenkogs naumenkogs mentioned this pull request Oct 13, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erlay status in getpeerinfo
5 participants