Skip to content
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

Ignore old format estimation file #11273

Merged
merged 1 commit into from Dec 19, 2017
Merged

Ignore old format estimation file #11273

merged 1 commit into from Dec 19, 2017

Conversation

murchandamus
Copy link
Contributor

The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

Pending testing.

@laanwj
Copy link
Member

laanwj commented Sep 7, 2017

Is this just a cleanup or does this result in crashes/misestimations?

maflcko
maflcko reviewed Sep 7, 2017
View reviewed changes
src/policy/fees.cpp Outdated
}
}
else { // nVersionThatWrote >= 149900
if (nVersionThatWrote > 149900) {
Copy link
Member

Choose a reason for hiding this comment

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

This will always evaluate to false, as nVersionThatWrote is currently ==149900

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so - on 0.15.0 it should be 150000 on master it should be 159900

Copy link
Member

Choose a reason for hiding this comment

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

Mixed that up as nVersionRequired, sorry.

@morcos
Copy link
Member

morcos commented Sep 9, 2017

@laanwj This is meant to just be cleanup. The code to process the old file was left in under the thought we might provide a transition, but then we never did. Which reminds me, we should mention in the release notes that upgrading starts your estimates over again.

@meshcollider meshcollider mentioned this pull request Sep 10, 2017
Merged
@TheBlueMatt
Copy link
Contributor

There are also similar cleanups to be had in TxConfirmStats::Read.

@murchandamus
Copy link
Contributor Author

@jnewbery and I tested this at CDT. It will only refrain from loading estimation files created by the previous version as they are not compatible with the new estimation file.

@jnewbery
Copy link
Contributor

@xekyo care to respond to @TheBlueMatt's comment? There are a couple of if statements in TxConfirmStats::Read that are now redundant:

bitcoin/src/policy/fees.cpp

Line 415 in 67879b7

if (nFileVersion >= 149900) {
and

bitcoin/src/policy/fees.cpp

Line 444 in 67879b7

if (nFileVersion >= 149900) {

This PR should also clean up that code.

@laanwj
Copy link
Member

laanwj commented Nov 30, 2017

Ping @xekyo , if you include @TheBlueMatt's nits this is ready for merge.

@laanwj laanwj changed the title WIP: Ignore old format estimation file Ignore old format estimation file Dec 19, 2017
@laanwj
Copy link
Member

laanwj commented Dec 19, 2017

utACK 3a3a9f9
I'm going to merge this. Removing this redundant code is clearly an improvement, and the other things suggested can be done later (added "up for grabs" tag).

@laanwj laanwj merged commit 3a3a9f9 into bitcoin:master Dec 19, 2017
laanwj added a commit that referenced this pull request Dec 19, 2017
@laanwj
Merge #11273: Ignore old format estimation file
cdd6bbf 
3a3a9f9 Ignore old format estimation file (Murch)

Pull request description:

  The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

  Pending testing.

Tree-SHA512: c8e3824dbdd8f6730133d5ad20b00995e9a63ab54431158a91e2f4d2aba5763b8aa698bce1fffca2713ba3a162e23d8fcd6e3efb9847b015c2e1e8725398150b
@promag
Copy link
Member

promag commented Dec 19, 2017

utACK cdd6bbf.

@TheBlueMatt TheBlueMatt mentioned this pull request Dec 19, 2017
Merged
laanwj added a commit that referenced this pull request Dec 20, 2017
@laanwj
Merge #11951: Remove dead feeest-file read code for old versions
1fb34e0 
62e7c04 Remove dead feeest-file read code for old versions (Matt Corallo)

Pull request description:

  0.15.0 introduced a new feeest file format, and support for parsing
  old versions was never fully added. We now simply fail to read the
  old format, so remove the dead partial-implementation.

  Follow up to #11273.

Tree-SHA512: c291ce51b7cb0c93479c18a1885dd646cbe03e4c7d12df03c0e99c0634e1bf9f9e41facf54a645227154bab58b9988f21b928cf3fc0520087c4eede4258c8861
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11273: Ignore old format estimation file
af1fce3 
3a3a9f9 Ignore old format estimation file (Murch)

Pull request description:

  The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

  Pending testing.

Tree-SHA512: c8e3824dbdd8f6730133d5ad20b00995e9a63ab54431158a91e2f4d2aba5763b8aa698bce1fffca2713ba3a162e23d8fcd6e3efb9847b015c2e1e8725398150b
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11951: Remove dead feeest-file read code for old versions
a22ab27 
62e7c04 Remove dead feeest-file read code for old versions (Matt Corallo)

Pull request description:

  0.15.0 introduced a new feeest file format, and support for parsing
  old versions was never fully added. We now simply fail to read the
  old format, so remove the dead partial-implementation.

  Follow up to bitcoin#11273.

Tree-SHA512: c291ce51b7cb0c93479c18a1885dd646cbe03e4c7d12df03c0e99c0634e1bf9f9e41facf54a645227154bab58b9988f21b928cf3fc0520087c4eede4258c8861
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11273: Ignore old format estimation file
7f431e3 
3a3a9f9 Ignore old format estimation file (Murch)

Pull request description:

  The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

  Pending testing.

Tree-SHA512: c8e3824dbdd8f6730133d5ad20b00995e9a63ab54431158a91e2f4d2aba5763b8aa698bce1fffca2713ba3a162e23d8fcd6e3efb9847b015c2e1e8725398150b
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11951: Remove dead feeest-file read code for old versions
1a57b6a 
62e7c04 Remove dead feeest-file read code for old versions (Matt Corallo)

Pull request description:

  0.15.0 introduced a new feeest file format, and support for parsing
  old versions was never fully added. We now simply fail to read the
  old format, so remove the dead partial-implementation.

  Follow up to bitcoin#11273.

Tree-SHA512: c291ce51b7cb0c93479c18a1885dd646cbe03e4c7d12df03c0e99c0634e1bf9f9e41facf54a645227154bab58b9988f21b928cf3fc0520087c4eede4258c8861
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11273: Ignore old format estimation file
b871cda 
3a3a9f9 Ignore old format estimation file (Murch)

Pull request description:

  The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

  Pending testing.

Tree-SHA512: c8e3824dbdd8f6730133d5ad20b00995e9a63ab54431158a91e2f4d2aba5763b8aa698bce1fffca2713ba3a162e23d8fcd6e3efb9847b015c2e1e8725398150b
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11951: Remove dead feeest-file read code for old versions
71ca6a3 
62e7c04 Remove dead feeest-file read code for old versions (Matt Corallo)

Pull request description:

  0.15.0 introduced a new feeest file format, and support for parsing
  old versions was never fully added. We now simply fail to read the
  old format, so remove the dead partial-implementation.

  Follow up to bitcoin#11273.

Tree-SHA512: c291ce51b7cb0c93479c18a1885dd646cbe03e4c7d12df03c0e99c0634e1bf9f9e41facf54a645227154bab58b9988f21b928cf3fc0520087c4eede4258c8861
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11273: Ignore old format estimation file
4dd3124 
3a3a9f9 Ignore old format estimation file (Murch)

Pull request description:

  The fee estimation data format changed from 0.14.x to 0.15.0, so we should no longer read the old data. H/T @jnewbery, @morcos

  Pending testing.

Tree-SHA512: c8e3824dbdd8f6730133d5ad20b00995e9a63ab54431158a91e2f4d2aba5763b8aa698bce1fffca2713ba3a162e23d8fcd6e3efb9847b015c2e1e8725398150b
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11951: Remove dead feeest-file read code for old versions
4a7a7cd 
62e7c04 Remove dead feeest-file read code for old versions (Matt Corallo)

Pull request description:

  0.15.0 introduced a new feeest file format, and support for parsing
  old versions was never fully added. We now simply fail to read the
  old format, so remove the dead partial-implementation.

  Follow up to bitcoin#11273.

Tree-SHA512: c291ce51b7cb0c93479c18a1885dd646cbe03e4c7d12df03c0e99c0634e1bf9f9e41facf54a645227154bab58b9988f21b928cf3fc0520087c4eede4258c8861
@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

@laanwj laanwj laanwj left review comments

@maflcko maflcko maflcko left review comments

Assignees
No one assigned
Labels
TX fees and policy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@murchandamus @laanwj @morcos @TheBlueMatt @jnewbery @promag @maflcko @fanquake