p2p: rpc: add tx_reconciliation to getpeerinfo #30984

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

    ---

Contributors
brunoerg DrahtBot sr-gi mzumsande

Labels
P2P