💬 maflcko commented on pull request "test: Use Decimal for fee calculations in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1706456465)
Yes, sounds good to leave the functions completely untouched in this pull. When internally `double` is changed to something else, the Python code can also use something other than `float` (which has double precision). Thanks for providing context.

💬 petertodd commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2272824670)
@ariard First of all, with ~100% of hash power mining full-rbf, `mempoolfullrbf=0` clearly provides no security benefit.

Users may have their own reasons to want a record of the first-seen transaction. But there's no reason why we in Core need to support such a niche use-case. It would make much more sense for users who need that functionality to get it directly, eg with some kind of "incoming transaction feed" hook.

For the use-case of zeroconf channels, I don't see how `mempoolfullrbf=0`
...

💬 TheCharlatan commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2272838226)
Concept ACK

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1706546530)
This was the result of a change that was reverted since, where it was important to test the exact before/after structure.

💬 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