💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2328519108)
macOS follow-up idea: Someone could check on macOS 13, if compilation with XCode still works. If not, `build-osx.md` could be updated (Ref: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716956530)

💬 fanquake commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1743507523)
It's disappointing that so many distros don't support CMake in their packaging. We basically can't switch this (and any other packages where we might want to use the "native" CMake config) until *every* distro does, otherwise it'll just be broken for that distro.

Some do, i.e [Alpine](https://pkgs.alpinelinux.org/contents?branch=edge&name=zeromq%2ddev&arch=x86&repo=main), or [Arch](https://archlinux.org/packages/extra/x86_64/zeromq/), or [nix](https://github.com/NixOS/nixpkgs/blob/release-23.
...

💬 GBKS commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2328544171)
Was just thinking that an alternative approach could be to leave the transactions list as is, but show a message in the send screen that explains the current status and trust assumptions. This is much more explicit than slightly tweaked icons. Just wanted to add it as an idea for an alternative approach, feel free to ignore for the purpose of this PR.

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743536308)
> Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499,

I don't quite think this is similar. My suggestion in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499 was to use less code to have more accurate and helpful error reporting, triggered only when that particular requirement is breached. In this PR, we're talking about adding more code to provide context that may not be relevant (and thus make
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2328592993)
Force-pushed to remove behaviour change in commit "rpc: use uint256::FromHex for ParseHashV", addressing @maflcko's [concerns](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1738970588) over behaviour change. I don't think this is necessarily an improvement, but I appreciate that's my subjective view and the (slight) behaviour change was orthogonal the goal of the PR anyway so it seems like the most pragmatic approach to make progress. Thanks for your review!

👍 marcofleon approved a pull request: "fuzz: Rename fuzz_seed_corpus to fuzz_corpora"
(https://github.com/bitcoin/bitcoin/pull/30804#pullrequestreview-2279780062)
ACK 8888beea8d477b1d4a2dfd2a0bb5f686de62f3ff

💬 remyers commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328622998)
> > Fwiw, I also ran across this problem (Ubuntu 20.04)
>
> I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with #29066 and shouldn't even pass the `cmake -B` command.

Yup, I have boost 1.74.0.3 installed.
```sh
dpkg -s libboost-dev | grep 'Version'
Version: 1.74.0.3ubuntu7
```

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2328685708)
> Concept ACK
>
> I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would be helpful. Maybe even similar to `getrawaddrman` and https://addrman.observer.

Thanks. That web app is gorgeous! Any particular tx details you'd like to see added (e.g. orphan expiration, etc.)?

👋 tdb3's pull request is ready for review: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793)

💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2328715995)
> Today's mempool does a suboptimal job of dealing with chain limits and RBFs. Assuming cluster mempool, we still have the issue of what to do with cluster limits that are exceeded even after removing RBF conflicts, if any. This has been called "sibling eviction" and in general seems doable in a DoS-resistant way but is left to future work. It's a problem I'm very interested in solving post-cluster mempool.

What I mean is, if you are doing an RBF replacement of one transaction with another tr
...

👍 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