💬 maflcko commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2272848626)
> I'm particularly interested if any protocol v2 nodes engage in it, since I think that's a viable way to immediately enforce without breaking anything.

Just for context, there is an idea for a "BIP324 proxy" for legacy light clients: https://delvingbitcoin.org/t/bip324-proxy-easy-integration-of-v2-transport-protocol-for-light-clients-poc/678 . However, it is experimental, only works for outbound connections, requires the light client to be patched right now (and may have other design issues)
...

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1706563939)
We've had something like that before: https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681077911

But it was ambiguous, as to whether it can fail or not during execution as we can see here:
https://github.com/hebasto/bitcoin/blob/master/src/pubkey.cpp#L345-L350 or https://github.com/hebasto/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L685

But @josibake argued that they cannot fail at that point - if they do it's a bug, but we didn't want to ignore the return values either.
...

💬 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 :)