💬 TheCharlatan commented on pull request "scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-2189915755)
Concept ACK

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1653528246)
> You might need to add a mechanism to mock the bdb reader class just so you can feed the migration process with a hand-crafted LegacyDataSPKM. This will let you avoid crafting the bdb file manually when bdb is not compiled anymore.

You're right, I'll move this PR to draft to work on it.

📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyScriptPubKeyMan` which can have some keys/HD keys/scripts/etc, and then migrate it to descriptor.

I tried to keep it as compatible as possible with future legacy wallet removal.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2189951185)
re: https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2090083022

AJ, thanks for the response. I want to let your comment stand, and not reply in detail right away to wait for input on this PR from other potential reviewers. I think your views on importance of hardcoding category constants in certain log statements, and on not allowing categories to be specified in other log statements are atypical, or at least will not be as strongly felt by other reviewers.

But your response does
...

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