👍 stickies-v approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2279873765)
ACK 6fa4c92e4c9bf5ea90e25d41b525c88f9c2d834d , modulo my lack of fuzzing experience to have a good view on whether the `fuzz/hex.cpp` changes indeed make the fuzz test better (the code looks correct and safe to me), as per https://github.com/bitcoin/bitcoin/pull/30618/files#r1742175853

I [maintain](https://github.com/bitcoin/bitcoin/pull/30618/files#r1742093199) that the `setup_common.cpp` changes should be dropped from dec011bdaaeb000a9f22a31e3fe9415ec7bf97dc, but it is not a blocker.

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743607635)
simplification nit
```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work, arith_uint256{0x1234});
```

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743620137)
> (though in this case that wouldn't even be true since `random_hex_string` can start with `0x` for `FromUserHex` which isn't added back via `ToString`)

Ah yes good point thanks, perhaps that would be good to clarify in a docstring?

> isn't the point of these tests that we shouldn't have know the internals (i.e. black box)?

I'm out of my depth here, so will probably refrain from commenting further and hopefully someone with more fuzzing experience can chime in. I think the point of the
...

👍 vasild approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2279921795)
ACK 5be6cc771faea00915a31240d9b5afdcdd4aa52f

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743546975)
```suggestion
ChainstateManager& m_chainman;
const CBlockIndex& m_invalidate_index;
public:
TemporaryRollback(ChainstateManager& chainman, const CBlockIndex& index) : m_chainman{chainman}, m_invalidate_index{index} {
InvalidateBlock(m_chainman, m_invalidate_index.GetBlockHash());
};
~TemporaryRollback() {
ReconsiderBlock(m_chainman, m_invalidate_index.GetBlockHash());
```

nit in e6812ffec338035a1ebe949015e95ba335c94d08: Doesn't change the behavior
...

👍 maflcko approved a pull request: "rpc: dumptxoutset height parameter follow-ups (29553)"
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2279769878)
review ACK e6812ffec338035a1ebe949015e95ba335c94d08 💦

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e6812ffec338
...

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743586494)
nit in e6812ffec338035a1ebe949015e95ba335c94d08: Wouldn't it be easier to just `throw` the error on creation, instead of creating a temporary symbol and check whether it is null and then `throw` it? With `TemporaryRollback` this should be possible now.

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743571147)
```suggestion
// process which may not be what the user wants.
```

nit in9467de52d3befd831b16b01c332793de18cb29cc : typo

Also "Don't do this if ..." can be replaced by "Skip if ...", but either is fine.

💬 instagibbs commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743671441)
I feel like the ability to fetch the whole transaction (set) optionally is useful, otherwise we just have a couple hashes of the transactions, no ability to estimate how many vbytes the orphanage is using, do any analysis on the data given?

Maybe have an optional argument via txid/wtxid that fetches the full serialized transaction in hex?

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743675879)
Agreed, will refinea

💬 instagibbs commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
Follow-up PRs can probably add more interesting fields as well, such as entry time, announcer peerid, etc.

I'll try not to scope creep this PR too bad

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743691588)
For originator, I was thinking `fromPeer` (the NodeId). Currently each orphan tracks 1 sender, but hoping to have multiple in the future (#28031 if you're curious).

For this PR, I think it would suffice to take an approach similar to `getrawmempool` but with an integer verbosity.
verbose=0: list of txids (it's ok if there are duplicates, it should be rare anyway)
verbose=1: list of objects with txid, wtxid, size, senders (a list of NodeIds which as of now has length 1), expiration

I thin
...

💬 hebasto commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328862764)
> Has this ever worked on your system? `guix shell: error` sounds like it is an error unrelated to Bitcoin Core?
>
> Can you use guix and guix shell normally?

Both command work just fine. For example:
```
$ guix package -i hello
$ guix shell python -- python3
```

Moreover, every other non-Darwin host is built successfully with `./contrib/guix/guix-build`.

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743697939)
> ability to estimate how many vbytes the orphanage is using

A `getorphanageinfo` can be added for overall stats, perhaps after #28031 which adds size tracking.

💬 fanquake commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328969074)
I think you'll need to debug this further. It's odd that building for all HOSTS (except one) would work, but building for `x86_64-apple-darwin` currently works fine, as far as I'm aware (just tested on two different systems).

> Both command work just fine. For example:

It's not clear that those are actually invoking the issue-causing behaviour. The error message points to `guix container`. i.e `guix shell --container`

🚀 fanquake merged a pull request: "fuzz: Rename fuzz_seed_corpus to fuzz_corpora"
(https://github.com/bitcoin/bitcoin/pull/30804)

💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329009795)
I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.

So, if anyone has any coins, which were received after block number 42335, then they
...

💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329027542)
> @sdaftuar do you think you could instrument your code to gather/dump clusters that linearize optimally with say 1,000,000 iterations but not using 100,000 iterations, using the code up to commit [21b826a](https://github.com/bitcoin/bitcoin/commit/21b826af07fdfa26aaf981cc0810edd6b55b45e0) (before that commit the search algorithm is so naive that high iteration count isn't really an indication of being hard).

I used my simulation environment to run the commit you listed on last year's data, s
...

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696)
style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.

🤔 maflcko reviewed a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280021001)
Concept ACK.

I left another question in the second commit. Sorry for the incremental review.