👍 tdb3 approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2141654158)
re ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
Thanks for implementing the changes.

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654751499)
> probably a bug fix.

In test-only code. But seems fine to fix, if the fix is correct.

💬 vasild commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#discussion_r1654817299)
This makes the call sites even more verbose and `LogInstance(), ` is repeated everywhere. In this case one improvement could be to use a default argument for that parameter which defaults to `LogInstance()` but I can see that this may not play well with the `...` arguments.

Maybe introduce another function that takes `(category, level, instance, ...)`? Or check if the first of `...` arguments in `(category, level, ...)` is a log instance and if not, then default the instance to `LogInstance()
...

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654843443)
re: https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654338332

> What is this lock used for?

Marking this thread resolved as the lock is now removed in later commit c8343e29a66082b37c37ee0ff8f7433b97eea9b1

🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2141863395)
Strange CI https://github.com/bitcoin/bitcoin/actions/runs/9649050504/job/26611651920?pr=26596 error.. it doesn't seem to be related. The "salvage" command opens the wallet in `DatabaseFormat::BERKELEY` format.

💬 willcl-ark commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2191727416)
@foolbear are you able to provide some concrete steps to reproduce this? I can't get it to happen in a few quick tests on my side.

Without reproduction steps it's likely going to be difficult to fix, and perhaps we should close the issue for now until we get verified repro steps.

👍 ryanofsky approved a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2141854539)
Code review ACK c8343e29a66082b37c37ee0ff8f7433b97eea9b1

Nice changes getting rid of some over-broad and recursive uses of cs_main.

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654849168)
In commit "Drop unneeded lock from createNewBlock" (1e4918b8f3af20577acdc9f201af44153e29bcb3)

Commit message could mention the reason the lock is not required in CreateNewBlock, which is just that CreateNewBlock already locks cs_main internally.

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654864264)
In commit "Have testBlockValidity hold cs_main instead of caller" (c8343e29a66082b37c37ee0ff8f7433b97eea9b1)

re: https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654687943

> `state` will be valid, no? So this may be confusing?

It seems like it would be better to do something like:


```c++
if (block.hashPrevBlock != tip->GetBlockHash()) {
state.Error("Block does not connect to current chain tip.")
return false;
}
```

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#issuecomment-2191753566)
It would be good to mention that the are some changes to cs_main locking (including a potential bugfix) in the PR description, otherwise this should be a refactoring with no changes in behavior.

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654954901)
thanks fixed in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654955419)
thank you fixed in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2191820659)
> Since calling `gettxoutsetinfo` with block hash depends on `coinstatsindex`. It would be simpler to address it on `feature_coinstatsindex`. It would be just one line, and would avoid the node restart and the function reordering.

That makes more sense I went ahead and did so in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654960188)
do you think I should still add this after f169562d85b332576b3a1cf32755e43023227ed9, rather than invalidating a block to then use `gettxoutsetinfo` with its hash I just used a random block hash. I can try doing this aswell

🚀 fanquake merged a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987)

👍 fanquake approved a pull request: "build: Drop redundant `sys/sysctl.h` header check"
(https://github.com/bitcoin/bitcoin/pull/30327#pullrequestreview-2142044015)
ACK c0b5ea5901d0ed005bca345e5b2de8a502f6af75 - we could got the other way, and add nested #defs, but that doesn't seem worthwhile.

🚀 fanquake merged a pull request: "build: Drop redundant `sys/sysctl.h` header check"
(https://github.com/bitcoin/bitcoin/pull/30327)

💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654994309)
nit: You could use named args here. It's better to understand what `"none"` and `True` are.

👍 BrandonOdiwuor approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2142120592)
Code Review ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e

🤔 theuni reviewed a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2142132960)
Post-merge utACK b5fc6d46a3854c18f6e8dfc89540d24ef778caa2