Fix (inverse) meaning of -persistmempool #23061

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2109-fixArgParse changing 4 files +9 −3
  1. MarcoFalke commented at 9:33 am on September 22, 2021: member
    Passing -persistmempool is currently treated as -nopersistmempool
  2. Fix (inverse) meaning of -persistmempool faff17bbde
  3. laanwj added the label Mempool on Sep 22, 2021
  4. laanwj added the label Needs backport (22.x) on Sep 22, 2021
  5. laanwj commented at 11:17 am on September 22, 2021: member
    Any idea when this regressed, or has this been the case since the option was introduced?
  6. MarcoFalke commented at 11:23 am on September 22, 2021: member
    I haven’t checked, but I remember seeing this bug (#23062) since I started working on Bitcoin Core, though I might be mistaken.
  7. laanwj added the label Needs backport (0.21) on Sep 22, 2021
  8. laanwj commented at 11:26 am on September 22, 2021: member
    Ok adding backport label for 0.21.x as well then. That seems far enough back.
  9. ryanofsky approved
  10. ryanofsky commented at 1:21 pm on September 22, 2021: member
    I think change of behavior for -persistmempool should be noted in release notes, but otherwise code review ACK faff17bbde6dcb1482a6210bc48b3192603a446f
  11. DrahtBot commented at 5:33 pm on September 22, 2021: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22976 (scripted-diff: Rename overloaded int GetArg to GetIntArg by ryanofsky)

    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.

  12. fanquake added the label Needs release note on Sep 23, 2021
  13. jnewbery commented at 1:52 pm on September 23, 2021: member

    Tested ACK faff17bbde6dcb1482a6210bc48b3192603a446f

    Verified that cherry-picking just the test change fails on master.

    Any idea when this regressed, or has this been the case since the option was introduced?

    Probably since it was introduced in #9966. Fortunately, I think it’s unlikely that anyone would be using -persistmempool since the default is true.

  14. MarcoFalke commented at 2:21 pm on September 23, 2021: member

    it’s unlikely that anyone would be using -persistmempool since the default is true.

    Indeed. Also, there is no bug report. So maybe we can skip the backport, as the bugfix is also a (silent) behaviour change.

  15. MarcoFalke force-pushed on Sep 23, 2021
  16. in doc/release-notes.md:91 in eeee1ac2ab outdated 
    85@@ -86,6 +86,12 @@ New settings
86 Updated settings
87 ----------------
88 
89+- The meaning of the command line option `-persistmempool` (without a value
90+  provided) incorrectly disabled mempool persistance. `-persistmempool` is now
91+  treated like other boolen options to mean `-persistmempool=1`.
    jonatack commented at 2:46 pm on September 23, 2021:
    nit, s/boolen/boolean/
    MarcoFalke commented at 6:55 am on September 24, 2021:
    Done
  17. in doc/release-notes.md:92 in eeee1ac2ab outdated 
    85@@ -86,6 +86,12 @@ New settings
86 Updated settings
87 ----------------
88 
89+- The meaning of the command line option `-persistmempool` (without a value
90+  provided) incorrectly disabled mempool persistance. `-persistmempool` is now
91+  treated like other boolen options to mean `-persistmempool=1`.
92+  `-persistmempool=0`, `-persistmempool=1` and `-nopersistmempool` are
    jonatack commented at 2:47 pm on September 23, 2021: 
    0  Passing `-persistmempool=0`, `-persistmempool=1` and `-nopersistmempool` is
    MarcoFalke commented at 6:55 am on September 24, 2021:
    Done
  18. jonatack commented at 2:48 pm on September 23, 2021: member
    Nice find.
  19. jnewbery deleted a comment on Sep 23, 2021
  20. jnewbery deleted a comment on Sep 23, 2021
  21. jnewbery deleted a comment on Sep 23, 2021
  22. jnewbery deleted a comment on Sep 23, 2021
  23. jnewbery deleted a comment on Sep 23, 2021
  24. jnewbery deleted a comment on Sep 23, 2021
  25. in doc/release-notes.md:90 in eeee1ac2ab outdated 
    85@@ -86,6 +86,12 @@ New settings
86 Updated settings
87 ----------------
88 
89+- The meaning of the command line option `-persistmempool` (without a value
90+  provided) incorrectly disabled mempool persistance. `-persistmempool` is now
    ryanofsky commented at 6:16 pm on September 23, 2021:
    s/persistance/persistence
    MarcoFalke commented at 7:52 am on September 25, 2021:
    Thanks, done
  26. in doc/release-notes.md:89 in eeee1ac2ab outdated 
    85@@ -86,6 +86,12 @@ New settings
86 Updated settings
87 ----------------
88 
89+- The meaning of the command line option `-persistmempool` (without a value
    ryanofsky commented at 6:19 pm on September 23, 2021:
    Maybe add “In previous releases,…” to be clear this is describing a bug in past releases.
    MarcoFalke commented at 7:52 am on September 25, 2021:
    Thanks, done
  27. MarcoFalke force-pushed on Sep 24, 2021
  28. MarcoFalke force-pushed on Sep 24, 2021
  29. jnewbery commented at 9:06 am on September 24, 2021: member
    ACK fa94851fcd
  30. MarcoFalke removed the label Needs release note on Sep 24, 2021
  31. ryanofsky approved
  32. ryanofsky commented at 4:36 pm on September 24, 2021: member
    Code review ACK fa94851fcd1636549c35bc7c3b42348a350d5741. (I thought I acked this previously, but my comments were stuck in draft. Feel free to ignore suggestions below which may be a little late)
  33. doc: Add 23061 release notes faa9c19a4b
  34. MarcoFalke force-pushed on Sep 25, 2021
  35. hebasto approved
  36. hebasto commented at 8:51 am on September 25, 2021: member
    ACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a, I have reviewed the code and it looks OK, I agree it can be merged.
  37. jnewbery commented at 8:02 am on September 27, 2021: member
    reACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a
  38. MarcoFalke removed the label Needs backport (0.21) on Sep 27, 2021
  39. MarcoFalke commented at 8:11 am on September 27, 2021: member

    Removed from 0.21 backport for now, because this is a behaviour change. It is not affecting anyone with high likelyhood.

    If it is backported to 22.1, the release notes can be cleared in master.

  40. MarcoFalke merged this on Sep 27, 2021
  41. MarcoFalke closed this on Sep 27, 2021
  42. MarcoFalke deleted the branch on Sep 27, 2021
  43. sidhujag referenced this in commit 8dba4ff338 on Sep 27, 2021
  44. luke-jr referenced this in commit 60ef97c3e8 on Oct 10, 2021
  45. fanquake referenced this in commit 45020413d6 on Oct 14, 2021
  46. fanquake referenced this in commit 976542e80f on Oct 14, 2021
  47. fanquake removed the label Needs backport (22.x) on Oct 14, 2021
  48. fanquake commented at 2:55 am on October 14, 2021: member
    Being backported to 22.x in #23276.
  49. fanquake referenced this in commit 8dc10a8540 on Oct 15, 2021
  50. fanquake referenced this in commit ed0cb59841 on Oct 15, 2021
  51. fanquake referenced this in commit bb1e8b0e6f on Oct 21, 2021
  52. fanquake referenced this in commit 5386e20489 on Oct 21, 2021
  53. luke-jr referenced this in commit e8bc4f35a2 on Dec 14, 2021
  54. fanquake referenced this in commit 19ef57355f on Feb 14, 2022
  55. fanquake referenced this in commit ccd261a479 on Feb 14, 2022
  56. fanquake referenced this in commit db76db7329 on Feb 15, 2022
  57. fanquake referenced this in commit 92d44ff36c on Feb 15, 2022
  58. fanquake referenced this in commit 9b5f674abb on Mar 1, 2022
  59. Fabcien referenced this in commit b969c04d06 on Sep 12, 2022
  60. DrahtBot locked this on Oct 30, 2022


MarcoFalke laanwj ryanofsky DrahtBot jnewbery jonatack hebasto fanquake

Labels
Mempool

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me