Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 7, 2022

It is not meaninful to run txorphanage without a node (validation, chainstate, mempool, rpc, ...). The module is in the node library and won't be added to the kernel library. Thus, it should be moved to src/node.

Also, move DEFAULT_MAX_ORPHAN_TRANSACTIONS to node/txorphanage.h, as it logically belongs to TxOrphanage::m_orphans and not to PeerManager.

Copy link
Contributor

@shaavan shaavan 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 dddd207

  • Verified that the scripted diff script works correctly, and the consequent diff is the same as the first commit.
  • Verified that the files are correctly sorted after the renaming.
  • I agree with converting const -> constexpr, which makes the variable a compile-time constant.

@maflcko maflcko force-pushed the 2207-txorphan-move- branch from dddd207 to fa6e073 Compare July 8, 2022 07:04
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK fa6e073

Changes since my last review:

  • Rebased over current master

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25667 (assumeutxo: snapshot initialization by jamesob)
  • #25527 ([kernel 3c/n] Decouple validation cache initialization from ArgsManager by dongcarl)
  • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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

@glozow glozow 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, light code review looks correct to me

Copy link
Contributor

@chinggg chinggg 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 fa6e073

  • Verified txorphan fuzz target works properly
  • I think it can make writing fuzz target easier by modularizing txorphanage

MacroFake added 2 commits July 19, 2022 09:21
-BEGIN VERIFY SCRIPT-
 # Move module
 git mv src/txorphanage.cpp src/node/
 git mv src/txorphanage.h   src/node/
 # Replacements
 sed -i 's:txorphanage\.h:node/txorphanage.h:g'     $(git grep -l txorphanage)
 sed -i 's:txorphanage\.cpp:node/txorphanage.cpp:g' $(git grep -l txorphanage)
 sed -i 's:TXORPHANAGE_H:NODE_TXORPHANAGE_H:g'      $(git grep -l TXORPHANAGE_H)
-END VERIFY SCRIPT-
@maflcko maflcko force-pushed the 2207-txorphan-move- branch from fa0096a to fa999c3 Compare July 19, 2022 07:22
@fanquake fanquake requested a review from ryanofsky July 19, 2022 07:42
@maflcko
Copy link
Member Author

maflcko commented Jul 19, 2022

Rebased

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK fa999c3

Changes since my last review:

  • Rebased over master.

@maflcko
Copy link
Member Author

maflcko commented Jul 26, 2022

Closing for now to open review for other pulls.

@maflcko maflcko closed this Jul 26, 2022
@maflcko maflcko deleted the 2207-txorphan-move-🚐 branch July 26, 2022 15:08
@bitcoin bitcoin locked and limited conversation to collaborators Jul 26, 2023
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.

6 participants