💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2270981318)
Force-pushed to address review comments, mainly:
- [fixed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1702863593) buggy `-noassumedvalid` behaviour and added unit tests to cover this
- [changed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1702863957) `result_size` to an int type and changed the disabled (default) state to `-1` instead of `0`
- [changed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199) `RANDOM_CTX_SEED` to now have to be a 64 c
...

🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2219106988)
Force-pushed to address review comments, mainly:
- [fixed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1702863593) buggy `-noassumedvalid` behaviour and added unit tests to cover this
- [changed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1702863957) `result_size` to an int type and changed the disabled (default) state to `-1` instead of `0`
- [changed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199) `RANDOM_CTX_SEED` to now have to be a 64 c
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1704245347)
Not sure about this. I don't find it more readable, and it also makes the code less robust to changes in other lines. Going to leave as is for now.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705281963)
Ugh, you're right, thanks a lot for catching this. This approach would've silently ignored `-noassumevalid` and `-assumevalid=0` by using the `defaultAssumeValid` value instead. I think it's not great that this bug didn't cause any tests to fail. `feature_assumevalid.py` doesn't catch this because `defaultAssumeValid` on regtest [is null](https://github.com/bitcoin/bitcoin/blob/43740f4971f45cd5499470b6a085b3ecd8b96d28/src/kernel/chainparams.cpp#L452) (and I don't think there's a way around this)
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705285352)
The `Number` part indicates that we're dealing with numerical values, where `0x00` means the same thing as `0x0000`, so I think that aligns with how the function behaves?
Return types are not immediately visible on in the callsite, and I think `Try` indicating that the function accepts malformed input is helpful for the reader, so I'm going to leave this as is.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1704248989)
Done (although `result_size=0` will always return `std::nullopt` because empty input strings are not allowed as per the existing `IsHexNumber` behaviour)

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1704179447)
Thanks, leftover from a previous version, updated now!

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705293768)
I couldn't see `RANDOM_CTX_SEED` be documented anywhere as having to be a 64 char hex string, so I thought trying to be lenient would be good for backwards compatibility, but I don't have a view on how important that is so happy to go with your simplification. Adopted in latest force push.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1704246877)
Sorry, leftover from previous version where it could be. Since `result_size` is now an `int` again, docs updated to say `Disabled if <= -1`

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1704246003)
Changed to an `int` type which takes a negative value to disable.

💬 jsarenik commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2270996435)
tACK 6bfa26048dba

💬 hebasto commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30573#issuecomment-2271043715)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/305.

🤔 paplorinc reviewed a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596#pullrequestreview-2221011614)
Thanks, please see if my comments would fit into the scope of this change

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705375642)
according to the bip the hrp `MUST contain 1 to 83 US-ASCII characters`, maybe we could extend the fuzz testing from the hard-coded "bc"

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705357479)
nit: According to https://en.bitcoin.it/wiki/BIP_0173 `bc` + `1` is `human-readable part` + `separator`, if we're splitting these up, maybe this would be a better documentation:
```suggestion
const auto hrp_size = 2, separator_size = 1;
if (input.size() + hrp_size + separator_size + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
```

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705377816)
nit: would it make sense to move these to the header as well - and make them static constexpr as well?
```suggestion
/** The Bech32 and Bech32m checksum size */
static constexpr size_t CHECKSUM_SIZE = 6;

/** The Bech32 and Bech32m character set for encoding. */
static constexpr const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";

/** The Bech32 and Bech32m character set for decoding. */
static constexpr const int8_t CHARSET_REV[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1
...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705382053)
👍, thanks for considering

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705386853)
ok, the number part can make sense, I'm still not sure about the `Try` part - seems like https://en.wikipedia.org/wiki/Hungarian_notation, which (as a Hungarian) I'm very much against :p

> Return types are not immediately visible on in the callsite

It's part of the method's signature, I don't see how repeating it in the method's name helps.
The parameters also aren't visible on the call site, yet we're not encoding them in the method's name.

If you insist, I won't block you on this of
...

🤔 darosior reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2220900524)
re-ACK 8208454eae7484d34a162ffa7701157e56e3cb80

Although i agree it's more readable now it's unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

That said i didn't find any issue and neither did my fuzzer (for now). I'll keep fuzzing this PR rebased on master for a day.

💬 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#discussion_r1705294173)
If we are going to rewrite stuff just for style purpose, i'll reiterate that this search is quadratic in the number of multipath values. Inserting each value in an unordered set and checking against it instead would make it linear in the number of multipath values.

That said i don't think it matters for any real world usage and therefore we should keep it for a follow-up.