🤔 furszy reviewed a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265#pullrequestreview-2231114653)
Code ACK 6b2dcba076
(https://github.com/bitcoin/bitcoin/pull/30265#pullrequestreview-2231114653)
Code ACK 6b2dcba076
📝 furszy opened a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621)
Blank legacy wallets do not have active SPKM. They can only be
detected by checking the descriptors' flag or the db format.
This enables the migration of blank legacy wallets in the GUI.
To test this:
1) Create a blank legacy wallet.
2) Try to migrate it using the GUI's toolbar "Migrate Wallet" button.
-> In master: The button will be disabled because `CWallet::IsLegacy()` returns false for blank legacy wallet.
-> In this PR: the button will be enabled and allow the legacy wa
...
(https://github.com/bitcoin/bitcoin/pull/30621)
Blank legacy wallets do not have active SPKM. They can only be
detected by checking the descriptors' flag or the db format.
This enables the migration of blank legacy wallets in the GUI.
To test this:
1) Create a blank legacy wallet.
2) Try to migrate it using the GUI's toolbar "Migrate Wallet" button.
-> In master: The button will be disabled because `CWallet::IsLegacy()` returns false for blank legacy wallet.
-> In this PR: the button will be enabled and allow the legacy wa
...
⚠️ Sheryl17 opened an issue: "Disclosure of remote crash due to addr message spam
Nodes could be spammed with addr messsages, which could be used to crash them. A fix was released on September 14th, 2021 in Bitcoin Core v22.0.
"
(https://github.com/bitcoin/bitcoin/issues/30622)
Nodes could be spammed with addr messsages, which could be used to crash them. A fix was released on September 14th, 2021 in Bitcoin Core v22.0.
"
(https://github.com/bitcoin/bitcoin/issues/30622)
✅ achow101 closed an issue: "Disclosure of remote crash due to addr message spam
Nodes could be spammed with addr messsages, which could be used to crash them. A fix was released on September 14th, 2021 in Bitcoin Core v22.0.
"
(https://github.com/bitcoin/bitcoin/issues/30622)
Nodes could be spammed with addr messsages, which could be used to crash them. A fix was released on September 14th, 2021 in Bitcoin Core v22.0.
"
(https://github.com/bitcoin/bitcoin/issues/30622)
:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30622)
(https://github.com/bitcoin/bitcoin/issues/30622)
🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2231282040)
> Why not allow 0x prefixes as -minimumchainwork does?
Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number. So we have to make a trade-off between not introducing "weirdness" into the code vs maintaining backwards compatibility (for an undocumented feature). I prefer leaning towards the former unless there are good reasons to go for the latter, but that's exactly the reason I carved out these commits into a smaller commit so thank you for your inpu
...
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2231282040)
> Why not allow 0x prefixes as -minimumchainwork does?
Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number. So we have to make a trade-off between not introducing "weirdness" into the code vs maintaining backwards compatibility (for an undocumented feature). I prefer leaning towards the former unless there are good reasons to go for the latter, but that's exactly the reason I carved out these commits into a smaller commit so thank you for your inpu
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712441877)
> Would be good to change "Size of" to "Desired size of".
"Desired" to me sounds like the result string may not actually be of the requested size, when it is actually guaranteed. I'm not sure that would be an improvement?
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712441877)
> Would be good to change "Size of" to "Desired size of".
"Desired" to me sounds like the result string may not actually be of the requested size, when it is actually guaranteed. I'm not sure that would be an improvement?
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712444215)
> I think instead of saying this "sanitizes" the string and pretending like that word means something it would be better to just say this strips 0x prefixes and fixes the length of the string so it can be passed to the base_blob::ParseHex() function.
Yeah, I felt a bit uneasy about the arbitrariness of what the function is doing and how it's named. I can't really come up with a better name, though, but I'm open to suggestions?
An alternative would be to, as @maflcko initially [suggested](h
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712444215)
> I think instead of saying this "sanitizes" the string and pretending like that word means something it would be better to just say this strips 0x prefixes and fixes the length of the string so it can be passed to the base_blob::ParseHex() function.
Yeah, I felt a bit uneasy about the arbitrariness of what the function is doing and how it's named. I can't really come up with a better name, though, but I'm open to suggestions?
An alternative would be to, as @maflcko initially [suggested](h
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712438144)
> I don't think this should be a standalone function when it is only called one place.
The reason I organized it this way is because I was quite surprised to see that we don't have unit testing on setting options from arguments, and `SetOptsFromArgs()` can trivially (when templated) be reused for any other options setting test, avoiding (non-move) changes to this file. With that said, would you still prefer not having a separate function, or e.g. templating it (prematurely) and moving it into
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712438144)
> I don't think this should be a standalone function when it is only called one place.
The reason I organized it this way is because I was quite surprised to see that we don't have unit testing on setting options from arguments, and `SetOptsFromArgs()` can trivially (when templated) be reused for any other options setting test, avoiding (non-move) changes to this file. With that said, would you still prefer not having a separate function, or e.g. templating it (prematurely) and moving it into
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712435380)
Thanks for your input. I wasn't sure about this either. See https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1703942199 for previous discussion on this topic. It seems example values have all been 64 characters in the past, and given that it's only used in tests it doesn't seem unreasonably to be a bit more strict to simplify the logic, but I appreciate your point of view too.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712435380)
Thanks for your input. I wasn't sure about this either. See https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1703942199 for previous discussion on this topic. It seems example values have all been 64 characters in the past, and given that it's only used in tests it doesn't seem unreasonably to be a bit more strict to simplify the logic, but I appreciate your point of view too.
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712432448)
> 0x which isn't mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.
`x`, whitespace, and trailing non-hex characters are all non-hex characters, so arguably it is mentioned in the commit message, but I agree it wouldn't hurt to be explicit about the 0x prefix and whitespace.
> and it also seems ok to not require numbers to be padded with leading 0's.
I'm a bit confused about this comment being in the `-assumeva
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712432448)
> 0x which isn't mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.
`x`, whitespace, and trailing non-hex characters are all non-hex characters, so arguably it is mentioned in the commit message, but I agree it wouldn't hurt to be explicit about the 0x prefix and whitespace.
> and it also seems ok to not require numbers to be padded with leading 0's.
I'm a bit confused about this comment being in the `-assumeva
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712427499)
> The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why 802374b4355bd1dec7a88bba6287c55f935699fe is a refactor commit?
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712427499)
> The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why 802374b4355bd1dec7a88bba6287c55f935699fe is a refactor commit?
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712440706)
Ah yes you're right, this is to follow existing behaviour of `IsHexNumber()` but it's not documented, I'll fix that, thanks.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712440706)
Ah yes you're right, this is to follow existing behaviour of `IsHexNumber()` but it's not documented, I'll fix that, thanks.
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888)
> In commit "test: Only accept a fully valid RANDOM_CTX_SEED" ([855784d](https://github.com/bitcoin/bitcoin/commit/855784d3a0026414159acc42fceeb271f8a28133))
>
> I don't understand the motivation for being so strict here.
Just for reference, you approved the exact same commit 2 days prior in https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242 .
Maybe my comment from https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706570743 can be moved into the commit m
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888)
> In commit "test: Only accept a fully valid RANDOM_CTX_SEED" ([855784d](https://github.com/bitcoin/bitcoin/commit/855784d3a0026414159acc42fceeb271f8a28133))
>
> I don't understand the motivation for being so strict here.
Just for reference, you approved the exact same commit 2 days prior in https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242 .
Maybe my comment from https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706570743 can be moved into the commit m
...
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2279822735)
> Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number.
If it's not a number, why are we prefixing it with 0 (by a method called TrySanitizeHex*Number*)?
> parameter of base_blob::FromHex(),
or alternatively chain the two together like `TrimAndParse`, i.e. `base_blob::FromHexLenient()`
> -assumedvalid [...] -minimumchainwork
Would it be clearer if we just warned about the changes in this release and require sanitized input in the next?
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2279822735)
> Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number.
If it's not a number, why are we prefixing it with 0 (by a method called TrySanitizeHex*Number*)?
> parameter of base_blob::FromHex(),
or alternatively chain the two together like `TrimAndParse`, i.e. `base_blob::FromHexLenient()`
> -assumedvalid [...] -minimumchainwork
Would it be clearer if we just warned about the changes in this release and require sanitized input in the next?
💬 Sjors commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2279837287)
Rebased after the (tiny) format change in #30598, for easier testing. Updated torrent link and snapshot file hash in the description. The chainparams themselves are unchanged.
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2279837287)
Rebased after the (tiny) format change in #30598, for easier testing. Updated torrent link and snapshot file hash in the description. The chainparams themselves are unchanged.
📝 paplorinc opened a pull request: "test: Fuzz the human-readable part of bech32 as well"
(https://github.com/bitcoin/bitcoin/pull/30623)
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
```
The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-12
...
(https://github.com/bitcoin/bitcoin/pull/30623)
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
```
The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-12
...
💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712594920)
Done in https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712594920)
Done in https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628
💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712595124)
Should I add this to https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628 as well?
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712595124)
Should I add this to https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628 as well?
💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2280810529)
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
Running a fuzzer for a couple days on the previous push (rebased on master to get the speedups from #30197) and with a dictionary specifically targeting the multipath expressions did not uncover anything.
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2280810529)
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
Running a fuzzer for a couple days on the previous push (rebased on master to get the speedups from #30197) and with a dictionary specifically targeting the multipath expressions did not uncover anything.