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.

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

backport: 24306, 24312, 24371 #6398

Merged

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

A few simple backports

What was done?

How Has This Been Tested?

Built locally; see CI

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

MarcoFalke added 2 commits November 17, 2024 17:49
…sable

60aa179 Use GetPathArg where possible (Pavol Rusnak)
5b946ed util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky)
687e655 util: Add GetPathArg default path argument (Ryan Ofsky)

Pull request description:

  Improve `ArgsManager::GetPathArg` method added in recent PR bitcoin#24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for bitcoin#24274 which is parsing more path options.

  - Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values.
  - Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated.
  - Add unit tests for default and negated cases
  - Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.

ACKs for top commit:
  w0xlt:
    Tested ACK 60aa179 on Ubuntu 21.10
  hebasto:
    re-ACK 60aa179

Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd
fa097d0 addrman: Log too low compat value (MarcoFalke)

Pull request description:

  Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.

  In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.

ACKs for top commit:
  mzumsande:
    re-ACK fa097d0

Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
@PastaPastaPasta PastaPastaPasta added this to the 22.1 milestone Nov 18, 2024
@UdjinM6
Copy link

UdjinM6 commented Nov 18, 2024

24306: pls consider 75c5043

24498: somehow 2030c69 is a completely different set of changes 🤷‍♂️

@PastaPastaPasta
Copy link
Member Author

Both should be resolved; somehow I ended up having the wrong commit message; weird!

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK ba7e500

@PastaPastaPasta PastaPastaPasta changed the title backport: 24306, 24312, 24498 backport: 24306, 24312, 24371 Nov 18, 2024
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

utACK ba7e500

@PastaPastaPasta PastaPastaPasta merged commit 018d80a into dashpay:develop Nov 18, 2024
27 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants