Skip to content

Conversation

@glozow
Copy link
Member

@glozow glozow commented Aug 29, 2023

This was taken from #28031 (see #27463 for project tracking).

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 29, 2023

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
ACK instagibbs, brunoerg, mzumsande, achow101
Concept ACK jonatack

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:

  • #28107 (util: Type-safe transaction identifiers by dergoegge)
  • #28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
  • #26697 (logging: use bitset for categories by LarryRuane)
  • #25038 (policy: nVersion=3 and Package RBF by glozow)

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.

Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 17e327d95ae9e55353f94acee126b227cf906c3f

modulo that one missing log, for completeness

@instagibbs
Copy link
Member

reACK 168db3b

glozow added 4 commits August 29, 2023 16:41
- Whenever a tx is erased. Allows somebody to see which transactions
  have been erased due to expiry/overflow, not just how many.
- Whenever a tx is added to a peer's workset.
- AcceptToMemoryPool when a tx is accepted, mirroring the one logged for
  a tx received from a peer. This allows someone to see all of the
  transactions that are accepted to mempool just by looking for ATMP logs.
- MEMPOOLREJ when a tx is rejected, mirroring the one logged for
  a tx received from a peer. This allows someone to see all of the
  transaction rejections by looking at MEMPOOLREJ logs.
This comment isn't in the right place, as detection of a tx in
recent_rejects would cause the function to exit much earlier.
Move the comment to the right place and tweak the first sentence for
accuracy.
@glozow glozow force-pushed the 2023-08-log-wtxids branch from 168db3b to a3b55c9 Compare August 29, 2023 15:41
@instagibbs
Copy link
Member

reACK a3b55c9

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK a3b55c9

Code looks great and m_recent_rejects check is done into AlreadyHaveTx so the local change makes sense for me. I just was in doubt if this comment could be in AlreadyHaveTx but seems ok to leave it in net_processing side.

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a3b55c9

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, happy to see improved or more useful logging.

@achow101
Copy link
Member

ACK a3b55c9

@achow101 achow101 merged commit 5666966 into bitcoin:master Aug 31, 2023
@glozow glozow deleted the 2023-08-log-wtxids branch August 31, 2023 17:14
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 10, 2024
Summary:
```
 - Additional orphan-related logging to make testing and debugging easier.
 - Add TXPACKAGES category for logging.
 - Move a nearby comment block that was in the wrong place.
```

Backport of [[bitcoin/bitcoin#28364 | core#28364]].

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16318
@bitcoin bitcoin locked and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants