💬 Fonta1n3 commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2189975986)
I think it should remove all bip32derivs.

My flow is like this:

Create a psbt with bip32derivs included, pass psbt to offline signer which signs inputs.

Pass signed psbt to walletprocesspsbt with bip32derivs set to false so that I can share the resulting psbt with other parties to create collaborative transactions, currently they would see my seed fingerprint and derivation paths for outputs which is not ideal.

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2189980086)
> Moved to draft for now, given at least one outstanding review comment and needs a rebase.

Thank you, sorry for the delay in addressing these changes. Currently my work schedule has completely overran my spare time.

I will carve out some time this week to rebase and refactor based on the review items.

Thanks

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

💬 1440000bytes commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2189989612)
I was not sure about .xyz domain for a bitcoin DNS seed based on things that I have read. Still ACKed it because I have been using .xyz domain for joinstr and had no issues. After recent fedimint incident I think `achownodes.xyz` will also get suspended sooner or later. So retracted my [ACK](https://github.com/bitcoin/bitcoin/pull/30007#pullrequestreview-2120313513).

DNS seed domains resolve to lot of IP addresses which may host some website. Its possible that someone will report it or it wil
...

👍 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?