💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668905168)
Thanks for the review and the C++17 suggestion @purpleKarrot.
If you agree with the changes, please add the latest hash after your ack, otherwise it's just a concept ack.

💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668914709)
ACK 7abc6e1f4b4aca4c8c576ff0cc27d68f90e42d28

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668927555)
Perhaps on a slow machine the timeout could go negative here:

```cpp
auto now{std::chrono::steady_clock::now()};
if (now >= deadline) break;
const MillisecondsDouble remaining{deadline - now};
block = miner.waitTipChanged(current_block.hash, remaining);
```

Though that seems unlikely, and I wouldn't expect it to happen consistently. In any case I added better handling for negative timeout.

💬 maflcko commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#discussion_r1961857705)
reject-details was only added in 221c789e91696569fa34dbd162d26e98cf9cab64 (master), so changing it seems fine

📝 darosior opened a pull request: "qa: clarify and document assumeutxo tests"
(https://github.com/bitcoin/bitcoin/pull/31907)
The `feature_assumeutxo.py` functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix those by making them less brittle and using clear, documented values.

I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic values were coming from, so i figured it was worth proposing this patch on its own for the sake of making the t
...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961867136)
I reproduced the CI bug and problem happens on this line. Looks like `block->hash` should be replaced by `current_block.hash` here and below

💬 maflcko commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961870266)
pls submit those upstream

🤔 l0rinc reviewed a pull request: "fuzz: a new target for the coins database"
(https://github.com/bitcoin/bitcoin/pull/28216#pullrequestreview-2626885053)
I still can't run any fuzzing on my Mac (for the past ~5 months), so I only added code review comments based on what I found.

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961863151)
The file name previously coincided with the only fuzz target - now a `coins_view` contains a `coins_db` target as well.
Given that the type is `CCoinsViewDB`, we could rename this to:
```suggestion
FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961742988)
The boolean parameter basically decides if the second parameter is a database view or not.
We could reduce this duplication by deducing it inside the `TestCoinsView` method instead:
```C++
void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view)
{
const bool is_db{!!dynamic_cast<CCoinsViewDB*>(&backend_coins_view)};
```
or if you prefer:
```C++
void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend_coins_view)
{
con
...

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961864110)
This is already obvious from two lines below:
```suggestion
.path = "",
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961749796)
nit: I know this was just copied over, but technically this is
```suggestion
.cache_bytes = 1 << 20, // 1MiB.
```

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961871432)
What's the purpose of the comment?
Why not add that to the commit message only?

💬 l0rinc commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1961727492)
I thought I commented on this already, but seems I forgot to submit:
👍 for
```suggestion
assert(is_db == !!coins_view_cursor);
```
or maybe even
```suggestion
assert(is_db != !coins_view_cursor);
```

💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2668954133)
> > > is to incorporate the following change.
> >
> >
> > Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.
>
> Yes, any other minor/test stuff I will leave for a follow-up.

A separate PR is fine. I raised this point because this PR introduces a way of using `CheckUnparsable` in a way that leads to some difficulty in reading the test.

🤔 rkrux reviewed a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2627150642)
Review @ 35d5adb

I reviewed range-diff
```
git range-diff bdc6ac1...35d5adb
```

Newer changes are fixing adding new assertions in the functional tests and improving comments.

I find the new error more detailed and helpful compared to the previous one.
```
➜ src git:(master) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
error code: -5
error message:
Multi: key ' L4rK1yDtCWekv
...

💬 glozow commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668959139)
> Alright, good to know. It might be noteworthy that there are other projects that directly forward Core's JSON (e.g. the Esplora implementations over at https://github.com/mempool/electrs/pull/105 and https://github.com/Blockstream/electrs/pull/119) that would also be affected by the API breakage

Ah right, I forgot about that. And https://github.com/spesmilo/electrumx/pull/288 and https://github.com/spesmilo/electrum-protocol/pull/1. I think this would inconvenience a few people.

Ok... ev
...

💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961881831)
Haven't noticed the header, will push a PR to https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h#L206

Should I remove it from here in the meantime?

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961882621)
Good catch, that's also consistent with `waitforblock`.

✅ maflcko closed an issue: "ERROR: AcceptBlockHeader: prev block not found"
(https://github.com/bitcoin/bitcoin/issues/31905)