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

💬 maflcko commented on issue "ERROR: AcceptBlockHeader: prev block not found":
(https://github.com/bitcoin/bitcoin/issues/31905#issuecomment-2668974005)
Your are running an EOL version of Bitcoin Core.

Is this still an issue with a recent version of Bitcoin Core?

💬 glozow commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961889923)
Another option would be to revert this.

💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1961892017)
I agree the TODO isn't very clear and possibly no longer applicable. Maybe it could be rounded up with other unclear comments and TODOs in a separate PR. A PR to just remove this one TODO line seems silly.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2627160751)
Updated 66c318abe1b7df3e2ff1035376bc5f4b2059626a -> 828c440b0dea29ab41ffc32d88969176d1286655 ([`pr/subtree.17`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.17) -> [`pr/subtree.18`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.17..pr/subtree.18)) fixing cmake whitespace.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961881829)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961422233

Good catch, fixed indentation and spacing. I don't think it should be too much of a burden to enforce a consistent style for cmake code given how little cmake code there is relatively. There also seems to be a [cmake-lint](https://cmake-format.readthedocs.io/en/latest/lint-usage.html) tool that could maybe help.

💬 hodlinator commented on issue "qa: timeout in StopHTTPServer()":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2668999318)
I'm working on making the code a bit more robust:
- Impose a timeout on HTTP connections shutting down - Maybe related to this issue.
- Fix an issue where we were not always processing the event to free eventHTTP - Would probably just leak otherwise, shutdown completes regardless of this in my testing.
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 88e640c377..ce2534fbd0 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -197,10 +197,15 @@ public:
return WIT
...

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide in help":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2668999401)
This is now based on #31785, and we now unhide all three wait methods.

📝 darosior opened a pull request: "Revert merge of PR #31826"
(https://github.com/bitcoin/bitcoin/pull/31908)
PR #31826 was merged [despite the code not compiling](https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638).

#31902 was opened to fix the code but since this code is only targeting a not officially supported platform, we don't have a CI in place to compile and run tests on this platform, neither apparently reviewers do (nor does the author?), don't take more risk right before 29 and revert the original broken PR.