Adds transaction propagation information to mempool transactions #32986

1. 
   sr-gi commented at 1:47 am on July 16, 2025: member

   Currently, we store the time transactions enter the mempool (in CTXMempoolEntry) as a timestamp with second precision.

   This patch stores data more precisely (microsecond granularity), as well as optionally storing the timestamp of the first inventory message received about each transaction in the pool, while still maintaining the previous interface for backwards compatibility (nTime is unchanged, and nTimeUs and nFirstInvTime are added with microsecond precision).

   This is really useful when measuring transaction propagation (e.g. in simulated networks) since there is no easy way of checking when a transaction was first announced or received by the node.

   I’ve tried to make the change as minimal as possible, hence why the use of an intermediate map in PeerManager instead of passing the time down to ProcessValidTx, which in turn would require adding it to ProcessOrphanTx, ProcessNewPackage, …

2. 
   DrahtBot commented at 1:47 am on July 16, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32986.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	stickies-v

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

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • transascation -> transaction [spelling error]
   • announcement (singular) -> announcements (plural) [subject–noun agreement]
   • remote it -> remove it [incorrect verb, should be “remove” rather than “remote”]

   drahtbot_id_4_m

3. 
   sr-gi commented at 1:50 am on July 16, 2025: member

   I’m currently seeking conceptual feedback. While I believe this is very useful for measuring transaction propagation and evaluating network improvements, I’m unsure whether the added complexity is justified, given that it’s unlikely to be used beyond that specific context.
4. DrahtBot added the label Needs rebase on Jul 16, 2025
5. 
   stickies-v commented at 12:49 pm on July 16, 2025: contributor

   I’m unsure whether the added complexity is justified, given that it’s unlikely to be used beyond that specific context.

   Would adding tracepoint(s) be a suitable alternative? More overhead for the user, but probably much less in this repo?

6. mempool: increases precision of reception times for CTxMemPoolEntries

   Instead of storing values in seconds, we stored them in microseconds. Also,
   adds a getter to retrieve nTime in microseconds.

   db95a25def
7. mempool: adds a nFirstInvTime to CTxMemPoolEntry

   nFirstInvTime stores the time the first announcement of the references
   transaction was received, if any. Also adds a getter method.

   7fa90d6a40
8. txrequest: adds m_first_inv to TxRequestTracker and reworks ReceivedInv

   Keeps track of the first received inv for a given transaction in txrequest.
   All matching txids will share the same first inv time.
   
   Also reworks TxRequestTracker::ReceivedInv to receive a recvtime and a delay
   instead of a reqtime(=recvtime + delay). This way, m_first_inv can be set
   to recvtime (when applicable), and reqtime can be computed internally instead of
   externally.

   4c18b9143e
9. txrequest: exposes GetFirstInvTime though the TxDownloadManager 6c19008f1b
10. net_processing, mempool: stores FirstInvTime in MempoolEntries

    Once a transaction make it to the mempool and before the TxDownloadManager
    information is cleared, update the FirstInvTime in the entry

    ab321a9161
11. mempool: updates mempool persist to account for new CTxMempoolEntry fields

    Stored nTimeUs instead of nTime, so both can be derived when loading, plus adds
    nFirstInvTime

    1737312b08
12. rpc: adds time_us and announcement_time to getmempoolentry 258cd6f9bf
13. sr-gi force-pushed on Jul 16, 2025
14. 
    sr-gi commented at 2:20 pm on July 16, 2025: member

    I’m unsure whether the added complexity is justified, given that it’s unlikely to be used beyond that specific context.

    Would adding tracepoint(s) be a suitable alternative? More overhead for the user, but probably much less in this repo?

    I think existing tracepoints can be used as the base to build something similar outside Core, like net:outbound_message and net:inbound_message. Similarly, a log grabber (grepper?) can also work in some cases. The issue I see with both solutions* is making it work at scale. When working with several hundred (or even close to thousands) nodes controlled from a single commander, these solutions have proven unreliable.

    * I haven’t personally tried building this using tracepoints, but I have by collecting logs and it becomes a mess.

15. 
    stickies-v commented at 3:02 pm on July 16, 2025: contributor

    I’m generally not in favour of making bespoke changes to production code or extending the public interface just to facilitate tests, monitoring, … if there exists a workaround, especially for temporary projects. In this case, after speaking with @sr-gi offline a bit more, it seems it seems like tracepoints, logging, small patchsets, … are feasible alternatives, so I’m Concept NACK on this one. (but I am open to making minimal changes to ensure the necessary erlay simulations can be ran)
16. DrahtBot removed the label Needs rebase on Jul 16, 2025
17. DrahtBot added the label CI failed on Jul 16, 2025
18. 
    DrahtBot commented at 4:28 pm on July 16, 2025: contributor

    🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/46101237165 LLM reason (✨ experimental): The CI failure is caused by a runtime error due to signed integer overflow in the standard chrono header during fuzz testing.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

19. 
    sr-gi commented at 6:33 pm on July 16, 2025: member

    I’m generally not in favour of making bespoke changes to production code or extending the public interface just to facilitate tests, monitoring, … if there exists a workaround, especially for temporary projects. In this case, after speaking with @sr-gi offline a bit more, it seems it seems like tracepoints, logging, small patchsets, … are feasible alternatives, so I’m Concept NACK on this one. (but I am open to making minimal changes to ensure the necessary erlay simulations can be ran)

    I think I agree with you. I may just maintain a smaller patch that can be applied to any branch for simulation purposes, even if it’s more hacky.

20. sr-gi closed this on Jul 16, 2025

    ---

Contributors
sr-gi DrahtBot stickies-v

Labels
CI failed