Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 12, 2021

The newly added wrapper is currently in the node library, but not placed in the node directory. While it is possible to use the wrapper outside of a node context (for example in a utility), it seems unlikely. Either way, I think the wrapper should either be moved to the util lib+dir or the node lib+dir, not something in-between.

Also, fix incorrect comment BITCOIN_DBWRAPPER_H.

MarcoFalke added 2 commits November 12, 2021 10:56
-BEGIN VERIFY SCRIPT-
 # Move module
 git mv src/minisketchwrapper.cpp src/node/
 git mv src/minisketchwrapper.h   src/node/
 # Replacements
 sed -i 's:minisketchwrapper:node/minisketchwrapper:g'     $(git grep -l minisketchwrapper)
 sed -i 's:MINISKETCHWRAPPER_H:NODE_MINISKETCHWRAPPER_H:g' $(git grep -l MINISKETCHWRAPPER_H)
 sed -i 's:DBWRAPPER_H:NODE_MINISKETCHWRAPPER_H:g'         ./src/node/minisketchwrapper.h
-END VERIFY SCRIPT-
@jnewbery
Copy link
Contributor

Concept ACK. Code tree organization should reflect library organization.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 13, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23517 (scripted-diff: Move miner to src/node by MarcoFalke)
  • #22362 (Drop only invalid entries when reading banlist.json by MarcoFalke)

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

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK faba1ab. I saw the comment in #21515, however given there hasn't been any new activity there, I'm going to merge this now.

@fanquake fanquake merged commit d092309 into bitcoin:master Nov 16, 2021
@maflcko maflcko deleted the 2111-libNode branch November 16, 2021 08:41
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Nov 17, 2022
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.

4 participants