🤔 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

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2191932755)
Force push was to squash all fixups (no other changes).

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655069069)
sounds good resolved in [6809ffd](https://github.com/bitcoin/bitcoin/pull/30340/commits/6809ffd8a6c589c515af48da2dd8367e4c6c2c26)

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655081131)
Missing:
```c++
#include <cstdint>
#include <chrono>
#include <array>
```

(Noticed while trying to forward-declare `netaddress.h`, which didn't work because of the `CService`s stored in `MappingResult`)

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655098222)
This header can be made dependency-free (while making `netaddress.h` a little more forward-declare-friendly) with:
```diff
diff --git a/src/netaddress.h b/src/netaddress.h
index 52fecada1c9..75d172ab334 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -31,3 +31,3 @@
*/
-enum Network {
+enum Network : int {
/// Addresses from these networks are not publicly routable on the global Internet.
diff --git a/src/util/netif.cpp b/src/util/netif.cpp
index ad3f93b379a..c4d4c3a31c
...