Rather than scale the memory usage with the total number attempts per tx to recompute necessary statistics, directly track the necessary information, leaving us with memory usage in-line with number of active peer connections and outstanding private relay txs.
Do note that this means per-peer statistics will be lost on that specific peer's disconnection.
Waiting for #35090 to be merged and I will rebase on top of it, but taking directional (N)ACKs now
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35680.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | optout21 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Rather than scale the memory usage with the total number
attempts per tx to recompute necessary statistics, directly
track the necessary information, leaving us with memory
usage in-line with number of active peer connections and
outstanding private relay txs.
159 | @@ -160,7 +160,9 @@ static RPCMethod getprivatebroadcastinfo() 160 | {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"}, 161 | {RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded transaction data"}, 162 | {RPCResult::Type::NUM_TIME, "time_added", "The time this transaction was added to the private broadcast queue (seconds since epoch)"}, 163 | - {RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction", 164 | + {RPCResult::Type::NUM, "num_sent", "Total number of peers this transaction was sent to so far"}, 165 | + {RPCResult::Type::NUM, "num_acknowledged", "Total number of peers that acknowledged reception of this transaction"}, 166 | + {RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction, for currently connected peers only",
If we make this change, is it worth reporting this? It will only have a value for max 3 minutes, and the received field will only ever have a value for a tiny amount of time that will require racing between receiving the PONG and disconnecting.
If we don't think we should store this data for stats a la #34707, we should remove these fields IMO. If we think they're valuable, we should instead cap this to last ~3 peers instead and just truncate the vector as we make more connections.
A limited buffer of disconnected peer stats, in addition to all the live ones, could make sense, if that's what you're suggesting
Concept ACK Made a light preliminary review (85d109a3ef8c42cb19075cd2b45a4daff072a711), LGTM. I note that unfortunately this PR has a significant overlap and strong conflict with #35016. I'm not sure what to recommend, since at this moment is hard to predict how they will land.
@optout21 wasn't aware of it, will review
wasn't aware of it, will review
👍 #35016 is no-behavior-change refactor, relatively non-contentious, however the PR moves a bit slowly. If it would be possible to change this PR to be closer to the approach there, that would be beneficial, but I'm not sure how the per-node map added here combines with the changes there.
From a glance, I'm not sure how that would work, they seem to be opposite tactics entirely? If the refactor was merged first, I'd have to undo most of it to reintroduce the wrapper types. Willing to be wrong, but I think we should pick a way forward and commit on strategy first.
CC: @vasild @andrewtoth
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.