p2p: rpc: add tx_reconciliation to getpeerinfo #30984

pull brunoerg wants to merge 5 commits into bitcoin:master from brunoerg:2024-09-erlay-getpeerinfo changing 4 files +15 −4
  1. brunoerg commented at 9:31 pm on September 26, 2024: contributor

    This PR adds a new field in getpeerinfo RPC to know whether a peer is registered for transaction reconciliation (we register it when receiving a SENDTXRCNCL msg).

    • The field tx_reconciliation is optional for in case you do not support tx reconciliation.
    • With this field, we can improve p2p_sendtxrcncl tests by checking whether a peer is registered for transaction reconciliation instead of simply rely on logs message.
    • d660df07d756ba5e06b74e4b51a7287620ae306b - Just remove unnecessary disconnect_p2ps (we’re calling it right before restarting the node) calls in p2p_sendtxrcncl.
  2. net_processing: add `m_tx_reconciliation` to `CNodeStateStats` 932e3f2268
  3. net_processing: add `reconcile_txs` to `PeerManagerInfo` 309cc3735e
  4. p2p: rpc: add `tx_reconciliation` to `getpeerinfo`
    We can use this field to know whether a peer is
    registered for transaction reconciliation.
    c4dec2ace3
  5. DrahtBot commented at 9:32 pm on September 26, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK sr-gi

    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:

    • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

  6. DrahtBot added the label P2P on Sep 26, 2024
  7. brunoerg commented at 9:32 pm on September 26, 2024: contributor
    cc: @sr-gi per our convo.
  8. brunoerg marked this as a draft on Sep 26, 2024
  9. brunoerg force-pushed on Sep 26, 2024
  10. DrahtBot added the label CI failed on Sep 26, 2024
  11. brunoerg force-pushed on Sep 26, 2024
  12. test: check `tx_reconciliation` from `getpeerinfo` in `p2p_sendtxrcncl`
    Instead of relying on log messages, check `tx_reconciliation`
    from `getpeerinfo` to ensure peer is or is not registered.
    eb20c09ae7
  13. test: remove unnecessary `disconnect_p2ps` calls in `p2p_sendtxrcncl` 1aa8206a48
  14. brunoerg force-pushed on Sep 27, 2024
  15. brunoerg marked this as ready for review on Sep 27, 2024
  16. DrahtBot removed the label CI failed on Sep 27, 2024
  17. sr-gi commented at 12:48 pm on September 27, 2024: member
    Concept ACK
  18. mzumsande commented at 2:55 pm on September 27, 2024: contributor
    This looks similar to #26602 / #27797 - I don’t think any Erlay PR has been merged since then, so the reasoning from back then should still apply?
  19. sr-gi commented at 3:04 pm on September 27, 2024: member

    This looks similar to #26602 / #27797 - I don’t think any Erlay PR has been merged since then, so the reasoning from back then should still apply?

    I was unaware of this. I’m happy to cherry-pick these two commits into one of the upcoming Erlay PRs, especially once the network messaging bits have been included.

  20. mzumsande commented at 3:11 pm on September 27, 2024: contributor

    I was unaware of this. I’m happy to cherry-pick these two commits into one of the upcoming Erlay PRs, especially once the network messaging bits have been included.

    Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.

  21. brunoerg commented at 3:19 pm on September 27, 2024: contributor

    This looks similar to #26602 / #27797 - I don’t think any Erlay PR has been merged since then, so the reasoning from back then should still apply?

    I completely forgot my own past PR haha. But one of the main motivation is to improve testing which currently relies on logs.

  22. brunoerg commented at 3:20 pm on September 27, 2024: contributor

    Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.

    Sounds good to me as well. Closing this for now.

  23. brunoerg closed this on Sep 27, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me