Skip to content

Refactor mempool.dat to be extensible, and store missing info #9422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
luke-jr wants to merge 2 commits into from
Closed

Refactor mempool.dat to be extensible, and store missing info #9422

luke-jr wants to merge 2 commits into from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Dec 25, 2016

Should fix #9103

@luke-jr luke-jr mentioned this pull request Dec 26, 2016
Closed
sipa
sipa reviewed Dec 27, 2016
View reviewed changes
@paveljanik
Copy link
Contributor

paveljanik commented Jan 8, 2017

Needs rebase.

sipa
sipa reviewed Jan 10, 2017
View reviewed changes
@luke-jr
Copy link
Member Author

luke-jr commented Jan 12, 2017

Fixed serialisation params. There are conflicts now - may I rebase?

@luke-jr luke-jr force-pushed the mempool_dat_extensible branch from 904552b to e98e4d2 Compare January 12, 2017 20:12
@luke-jr
Copy link
Member Author

luke-jr commented Jan 12, 2017

Rebased.

@luke-jr luke-jr force-pushed the mempool_dat_extensible branch 2 times, most recently from 93e033d to b14300f Compare February 6, 2017 19:30
kallewoof
kallewoof reviewed Feb 10, 2017
View reviewed changes
Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

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

utACK b14300f

src/validation.cpp
try {
uint64_t version;
file >> version;
if (version != MEMPOOL_DUMP_VERSION) {
Copy link
Contributor

@kallewoof kallewoof Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The implications seem to be minor, but this means all bitcoin nodes prior to this PR being merged will drop all their mempools on startup. Is that okay? Would it be possible / worth it to load version=1 mempools too? Above comment by @sipa seems to indicate this was never used, in which case I think we should simply say MEMPOOL_DUMP_VERSION = 1 above.

@luke-jr luke-jr force-pushed the mempool_dat_extensible branch from b14300f to 5d25b27 Compare August 23, 2017 18:43
@luke-jr luke-jr force-pushed the mempool_dat_extensible branch from 5d25b27 to 8d309b8 Compare September 3, 2017 06:23
TheBlueMatt
TheBlueMatt reviewed Nov 10, 2017
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

I'm not actually sure we want to save mempoolminfee - if you just restarted the practical mempoolminfee on the network may be much lower....it puts you in a state of downloading lots of transactions only pretty briefly (assuming any of your peers aren't limiting their mempool, which they almost always are, so mempoolminfee on most nodes never actually leaves 0 anyway). Probably not worth a incompatible version just for this, IMO, though if we had other things we wanted to store we could revisit.

@sipa
Copy link
Member

sipa commented Mar 6, 2018

I'm not sure this is worth it right now; we can revisit if there is critical information to add to the mempool file?

@luke-jr luke-jr force-pushed the mempool_dat_extensible branch from 8d309b8 to 1d34442 Compare March 6, 2018 19:20
@laanwj
Copy link
Member

laanwj commented May 14, 2018

Thee seems to be no agreement to do this right now.
I'm also going to close #9103, we shouldn't have that issue open if we don't agree that this should be done.

@laanwj laanwj closed this May 14, 2018
@laanwj laanwj mentioned this pull request May 14, 2018
Closed
@luke-jr luke-jr mentioned this pull request Dec 20, 2018
Closed
@luke-jr luke-jr mentioned this pull request Jul 11, 2020
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

+3 more reviewers

@kallewoof kallewoof kallewoof left review comments

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

@dcousens dcousens dcousens approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mempool saving doesn't save mempoolminfee

8 participants

@luke-jr @paveljanik @sipa @laanwj @kallewoof @dcousens @TheBlueMatt @fanquake