👍 hebasto approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2139898345)
re-ACK cc58e958f341d2759fbabe5c9d8cc557e17d587f.

🤔 ariard reviewed a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132#pullrequestreview-2139937422)
Tested ACK `512278e`

Manual mutation testing with following diff:

```
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index 4e1e24a11d..0850b2e60e 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -22,7 +22,7 @@ static constexpr unsigned int DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB{5};
/** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
static constexpr unsigned int DEFAULT_MEMPOOL_EXPIRY_HOURS{3
...

💬 AngusP commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2190051885)
(late) Code Review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7

Also this was good to read to get more familair with the code touched, thanks!

💬 achow101 commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2190116382)
> After recent fedimint incident I think `achownodes.xyz` will also get suspended sooner or later.

That is a risk of using anything DNS as the registry operator can seize them. But I think it's preferable to have more DNS seeders so that we are not overly reliant on a few of them. Furthermore, DNS seeders are not critical infrastructure - one being shut down does not really impact the network that much. They're also easily portable and changing domains is not difficult.

> DNS seed domains
...

👍 tdb3 approved a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2140135643)
ACK 22fe97a5cc612f438ec6280757487934501bf527

Changes look straightforward. Took a quick look at the other interface functions in `mining.h`, and the function arguments look to all now be in the in/inout/out order. Also manually ran `getblocktemplate` RPC on signet and on regtest (after broadcasting a few transactions). All looked well.

🤔 hebasto reviewed a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2140169218)
My Guix build:
```
x86_64
aa0ac90d4abc930dee2327985fafdbee9c40c8eeea5738383e798f076db36766 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/SHA256SUMS.part
c25dec340e71ba13b238cc743cb86cb6ee564b05f47a1d3df2240f90b8cdf1cd guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu-debug.tar.gz
b7e9627b89d06e2646fcc5430f2eb08ab920039cfa9d4ce14917830bc48ba129 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu.tar.gz
3afe6f97b
...

💬 tdb3 commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653781334)
nit: If this file is touched, would recommend using `first_added_node` or `added_node_0` instead of `added_node` to make it clearer that the node being referred to is the one added before 11.22.33.44. Alternatively, could update the comment/log message instead.

💬 tdb3 commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653782022)
nit: How about making this comment a log message instead?

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice

> Instead of inline comments or no test documentation at all, log the comments to the test log, e.g. self.log.info('Create enough transactions to fill a block'). Logs make the test code easier to read and the test logic easier [to debug](https://github.com/bitcoin/bitcoin/blob/master/test/README.md#test-logging).

Also, since we're
...

🤔 tdb3 reviewed a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2140194570)
Approach ACK

Good update to exercise the filtering feature of the RPC.
Left a suggesetion and nit.

💬 tdb3 commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653788093)
Manually induced a failure here by using `getaddednodeinfo(node="192.168.99.99")`. Test failed as expected.

📝 kevkevinpal opened a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340)
#### Description
There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.

You can see there are no tests that do the following by doing the below
`grep -nri "Block not found.**gettxoutsetinfo" ./test/functional/`

which leads to no results

---

#### Note:
I moved the order of when `self._test_gettxoutsetinfo` is called because at the end we restart the node with `-coinstatsindex=1` b
...

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653823679)
Made the change.

💬 jsarenik commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2190512170)
Tested ACK aba504759caa

🤔 kevkevinpal reviewed a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2140417645)
Concept ACK [13a871f](https://github.com/bitcoin/bitcoin/pull/30339/commits/13a871f8e3835355b84ba8a404870dcb33441aef)

ran locally and it passed for me

💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653913528)
I'm not sure if we test for any list of added_nodes greater than a length of 1

💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653905703)
instead of modifying the code here why not use `self.nodes[0].addnode(node="11.22.33.44", command='remove')` instead?

💬 maflcko commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1654199663)
> The actual code change is very simple

If it was so simple, then it would be good to explain how it could possibly work at all. Let me know what I am missing:

> However, this change may lead to problems. For example, does yaml allow duplicate keys? If yes, how are they handled? Does the skip via FILTER_TEMPLATE override this one, or vice versa? Will it change in the future?

See also "yaml duplicate keys" in your favourite search engine.

👍 maflcko approved a pull request: "Fix issues with CI on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2140849311)
lgtm ACK

My preference would be to have the `.cirrus.yml` as single source of ground truth. However, requiring forks to update this file is too cumbersome, so this alternative seems fine.

(The "config resolution strategy" is another setting outside the yaml that needs to be modified to "merge for prs", which could be documented in a follow-up)

But this looks good in any case.

💬 Sjors commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1654215983)
It's conceptually simple, but I agree that we shouldn't use duplicate keys unless Cirrus docs explicitly says you can. I could work around it in various ways, but for now I found it easier to just drop the commit.

💬 maflcko commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2190935028)
> Optionally set NO_BRANCH=true ~and NO_ARM=true~ env variables (see .cirrus.yml)

This is wrong in the description. The name was changed.