rpc, net: add erlay status in getpeerinfo #27797

1. 
   brunoerg commented at 3:03 pm on May 31, 2023: contributor

   Fixes #26602

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

2. net, rpc: add Erlay status in `getpeerinfo`

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

   8c03db83ab
3. test: `tx_reconciliation` field in `getpeerinfo` beeb7e3006
4. 
   DrahtBot commented at 3:03 pm on May 31, 2023: contributor

   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.

5. 
   brunoerg commented at 3:03 pm on May 31, 2023: contributor

   cc: @Sjors
6. 
   Sjors commented at 3:20 pm on May 31, 2023: member

   Concept ACK
7. 
   mzumsande commented at 3:33 pm on May 31, 2023: 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).
8. 
   Sjors commented at 3:38 pm on May 31, 2023: member

   @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.
9. 
   mzumsande commented at 3:57 pm on May 31, 2023: 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.

10. 
    brunoerg commented at 4:25 pm on May 31, 2023: contributor

    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.
11. brunoerg marked this as a draft on May 31, 2023
12. 
    fanquake commented at 9:19 am on June 1, 2023: member

    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.
13. fanquake closed this on Jun 1, 2023

    ---

14. bitcoin locked this on May 31, 2024

Contributors
brunoerg DrahtBot Sjors mzumsande fanquake