🤔 mzumsande reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2139518716)
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

I'd prefer combining some of the first commits with the respective test commits that actually make use of the change, but that's just my opinion ad maybe a matter of taste.

💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1653358992)
looks good to me now!

👍 rkrux approved a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2139514584)
tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae)

`make, test/functional are successful`.

From what I understand about Silent Payments, the calculation of the recipient address is dependent on the input hash for which it makes sense to push down the `txoutputs` addition _after_ coin selection.

💬 rkrux commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1653355957)
Although being a bit pedantic, but as this commit is supposed to be move-only, the removal of `CTxOut txout...` in line 1107 and the replacement of `push_back` with `emplace_back` in line 1115 could have been part of the previous commit to avoid any confusion for future readers.

💬 romanz commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1653368609)
Thanks - fixed in 24e7925abb.

💬 TheCharlatan commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2189768540)
Guix builds (aarch64):
```
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
4e3ea3b82
...

📝 brunoerg opened a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339)
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.

💬 TheCharlatan commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2189810744)
Concept ACK

Re https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2090083022

> Doing that meaningfully means changing all the logging in all the bitcoin-kernel related modules (eg validation, mempool, blockstorage, scheduler, ...) to explicitly specify a logger rather than using the global functions, which seems like a lot of pointless busy work? I can't imagine why we'd want to support having multiple bitcoin-kernel instances in a single executable logging to different loggers; if
...

👍 willcl-ark approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2139666308)
re-ACK 3ab25201909bece9066ac6191670bcee09791d54

Based on range diff since previous review: `git range-diff a2fc9ddee9cbaeffb3c460973bab3c2dd734db55...3ab25201909bece9066ac6191670bcee09791d54`

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653457550)
It doesn't quite work, because it would return:
```
fs::path GetDefaultDataDir()
{
// Windows:
// old: C:\Users\Username\AppData\Roaming\Bitcoin
// new: C:\Users\Username\AppData\Local\Bitcoin
// macOS: ~/Library/Application Support/Bitcoin
// Unix-like: ~/.bitcoin
```
but we are looking for:
```
// Windows: C:\Users\Satoshi\
// macOS: /Users/satoshi/
// Unix-like: /home/satoshi/
```

💬 willcl-ark commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2189839521)
Hi @Fonta1n3

I've taken a look at this , but in making a few changes one thing I wasn't sure of from a design perspective, was whether it would be more (or less) deisrable to have the `bip32derivs` bool always toggle _both_ input and output derivations, or might be preferable to have an option for doing each independently?

I think having them both effected by the same option probably makes more sense (in terms of what a user would "expect"), and I think it would actually still be possible
...

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653478731)
> It doesn't quite work, because it would return:
> ...
> // Unix-like: ~/.bitcoin

Don't trust the comment, it gives me the absolute path. Tested on Linux with:
```C++
const fs::path absolutePath = GetDefaultDataDir() / fs::path("specialWallet") + fs::path::preferred_separator;
```

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