💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706570743)
> 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.

Thanks.

For historic context, it was added in fae43a97ca947cd0802392e9bb86d9d0572c0fba, where it is only printed. Thus, the only source should be to copy-paste from a printed value.

If not, and some
...

👍 maflcko approved a pull request: "ci: Silent Homebrew's reinstall warnings"
(https://github.com/bitcoin/bitcoin/pull/30591#pullrequestreview-2223236574)
lgtm

💬 Shuhaib07 commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2272899107)
Merge and get approved

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780)
Thanks for covering these scenarios!

I checked to see how this fails for the previous error:
> unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access

i.e. it doesn't get to the `IsNull` check since it can't call `value()` on a null optional, right?

I think we can work around that by comparing the dereferenced optional instead:
```suggestion
BOOST_CHECK_EQUAL(*set_opts({"-assumevalid=0"}).assum
...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706639474)
nice

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706647454)
I don't particularly like the alternative, but the developer notes state:
```
- Use a named cast or functional cast, not a C-Style cast. When casting
between integer types, use functional casts such as `int(x)` or `int{x}`
instead of `(int) x`. When casting between more complex types, use `static_cast`.
```

🤔 paplorinc reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2223466412)
Will continue reviewing a bit later, I only had time for these so far

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706642642)
nit: my first reaction was here - "I wonder if there's an off-by-one error here".
If you think it makes sense, consider this alternative:
```suggestion
if (result_size < 0) result_size = std::numeric_limits<int>::max(); // negative result_size disables the size check
```

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706662216)
> > has failed [000000016bcc2e20000000016bcc2e20000000016bcc29a03b66800104140f00 != 0000000000000000000000000000000000000000000000000000000000000000]
>
> Which I find a lot more revealing.

Can you run this in valgrind? Dereferencing a nullopt is UB (undefined behavior), which must be avoided in production (obviously), but also in tests.

If you want to learn which methods expose undefined behavior, I find [https://en.cppreference.com/w/cpp/utility/optional/operator*] a good resource.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-2273008031)
Follow-up ideas for this pull requests are:

* The linearize scripts should be adjusted to apply any XOR, if it exists. C.f https://github.com/bitcoin/bitcoin/pull/28052/files#diff-cabb34205d48861dbb53b2ad0df92776bf7d917150caf17778e15fbc8e63e123R30
* A new test for undo data can be written: https://github.com/bitcoin/bitcoin/pull/28052/files#r1597462109

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706674268)
That wouldn't work. `result_size` is used both to throw if the input is too large, as well as to pad with leading zeroes if it's too short: https://github.com/bitcoin/bitcoin/pull/30569/files#diff-3f688af8f182edecd9c33977b905b3e71dc010574f721fd4e328bdbc7706f574R59

💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677297)
done

💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677230)
done

💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677702)
right of course! done, thanks

💬 maflcko commented on pull request "doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version":
(https://github.com/bitcoin/bitcoin/pull/30580#issuecomment-2273019342)
review-only ACK ed83974bb411ab5ebe3eef28f0ac995ce07936cd

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706681205)
Thanks, will change to `int{x}` in next force push.

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706688706)
Pretty sure there are two separate bugs here:

* The first is that `BOOST_NO_CXX98_FUNCTION_BASE` isn't removed from `CMAKE_REQUIRED_DEFINITIONS` after this check, which means it's incorrectly reused for the check below.

* The second is that when `-DWERROR=ON` is used, `-Werror` isn't being passed to the check for the Boost Test header (which also hides the first bug).

💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273059079)
Friendly ping @stickies-v @maflcko :)

💬 ajtowns commented on issue "RFC: ArgsManager type and range checking":
(https://github.com/bitcoin/bitcoin/issues/22978#issuecomment-2273070206)
> Posting to stop thinking about this for now, but I started working on a change that lets you declare type-safe settings without global constants, in a way that should be compatible with options structs and standalone setting retrieval.

Seems okay, though having a global type name that contains data (`UpnpSetting::Register` and `UpnpSetting::Update` have `summary.value` and `help.value`) doesn't seem meaningfully different to a global constant to me.

> This idea is to declare settings typ
...

💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1706731123)
> I think a better approach is to introduce flags which are internally coherent and actually support features we know we will use, and then roll the flags slowly, to one setting at a time, changing behavior of each setting once rather than multiple times, and documenting changes explicitly.

I agree, but I don't think it is incompatible with my suggestion. It seems possible to just implement bool error checking (and only that, or any other single feature), and make sure it is coherent and will
...

💬 maflcko commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273096799)
CI is green and the diff looks reasonable, so I guess it is harmless and fine.

Did you try setting the source dir read-only, and it passes with this change?