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

👍 pablomartin4btc approved a pull request: "cmake: Exclude generated sources from translation"
(https://github.com/bitcoin/bitcoin/pull/31899#pullrequestreview-2627202385)
tACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03

Tested in both Ubuntu and macOS.

💬 mzumsande commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2669011985)
> Is this not where the chainstate related changes are flushed to disk?
> https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069

The call uses `FlushStateMode::IF_NEEDED`, which means it usually won't do anything - it will only flush if it's necessary because the cache has grown to a critical size.

💬 darosior commented on pull request "random: move VerifyRNDRRS above InitHardwareRand":
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2669021440)
I think for now it's safer to just revert the original PR https://github.com/bitcoin/bitcoin/pull/31908. The changes can be re-considered and re-reviewed after the 29.0 branch-off.

✅ tnull closed a pull request: "Fix field name styling in `submitpackage` RPC results"
(https://github.com/bitcoin/bitcoin/pull/31900)

💬 tnull commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2669032754)
> Ok... even though it's marked as experimental, I think it's too late to break `submitpackage` purely for style reasons. Fwiw I did think this was worthwhile before (and I pinged @tnull after seeing [comment](https://github.com/spesmilo/electrum-protocol/issues/4#issuecomment-2665116596)), but I'm leaning towards nack now. My apologies!

Alright, no worries! Going ahead and closing this.

💬 glozow commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669053075)
Up to reviewers to decide whether this or #31902 is more appropriate, but added to milestone so we do this before branch off.