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.

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

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@luke-jr
Member

luke-jr commented Dec 25, 2016

Should fix #9103

@@ -4027,7 +4027,7 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
return VersionBitsStateSinceHeight(chainActive.Tip(), params, pos, versionbitscache);
}
static const uint64_t MEMPOOL_DUMP_VERSION = 1;
static const uint64_t MEMPOOL_DUMP_VERSION = 2;

This comment has been minimized.

@sipa

sipa Dec 27, 2016

Member

We never released a version with version 1, I think?

@sipa

sipa Dec 27, 2016

Member

We never released a version with version 1, I think?

This comment has been minimized.

@luke-jr

luke-jr Dec 27, 2016

Member

I'm planning to for Knots 0.13.2

@luke-jr

luke-jr Dec 27, 2016

Member

I'm planning to for Knots 0.13.2

This comment has been minimized.

@sipa

sipa Jan 10, 2017

Member

Ok. There is hardly any loss...

@sipa

sipa Jan 10, 2017

Member

Ok. There is hardly any loss...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jan 8, 2017

Contributor

Needs rebase.

Contributor

paveljanik commented Jan 8, 2017

Needs rebase.

@@ -4027,7 +4027,7 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
return VersionBitsStateSinceHeight(chainActive.Tip(), params, pos, versionbitscache);
}
static const uint64_t MEMPOOL_DUMP_VERSION = 1;
static const uint64_t MEMPOOL_DUMP_VERSION = 2;

This comment has been minimized.

@sipa

sipa Jan 10, 2017

Member

Ok. There is hardly any loss...

@sipa

sipa Jan 10, 2017

Member

Ok. There is hardly any loss...

Show outdated Hide outdated src/validation.cpp
@@ -4092,24 +4113,43 @@ bool LoadMempool(void)
return true;
}
template <class T>
std::vector<unsigned char> SerializeToVector(T o) {
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);

This comment has been minimized.

@sipa

sipa Jan 10, 2017

Member

SER_DISK / CLIENT_VERSION? I doubt it matters for anything, but better be consistent for now.

@sipa

sipa Jan 10, 2017

Member

SER_DISK / CLIENT_VERSION? I doubt it matters for anything, but better be consistent for now.

Show outdated Hide outdated src/validation.cpp
it = mapData.find("deltas");
if (it == mapData.end()) {
try {

This comment has been minimized.

@sipa

sipa Jan 10, 2017

Member

Can't this try block be made to enclose all deserialization operations, so that the catch and error reporting can be written only once?

@sipa

sipa Jan 10, 2017

Member

Can't this try block be made to enclose all deserialization operations, so that the catch and error reporting can be written only once?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 12, 2017

Member

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

Member

luke-jr commented Jan 12, 2017

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 12, 2017

Member

Rebased.

Member

luke-jr commented Jan 12, 2017

Rebased.

@@ -4192,42 +4192,63 @@ bool LoadMempool(void)
if (version != MEMPOOL_DUMP_VERSION) {

This comment has been minimized.

@kallewoof

kallewoof Feb 10, 2017

Member

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.

@kallewoof

kallewoof Feb 10, 2017

Member

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.

@TheBlueMatt

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 6, 2018

Member

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

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?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 14, 2018

Member

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment