🚀 achow101 merged a pull request: "random: add benchmarks and drop unnecessary Shuffle function"
(https://github.com/bitcoin/bitcoin/pull/30396)

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2218807473)
http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-30413/harnesses/p2p_handshake/coverage_report/coverage/workdir/bitcoin/src/net_processing.cpp.html#L3702

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1671297840)
done. Performance doesn't seem that important, but I also think it's a bit clearer that way.

🤔 tdb3 reviewed a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2167594504)
I think this is really close. Left one more comment.

💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1671300188)
Unless I'm overlooking something, might be able to simplify/shorten the change by calling `getaddrinfo()` again when `n_err` is non-zero. Would avoid the lambda function and might also get rid of the standalone `ai_flags`.

Something like:

```c++
addrinfo* ai_res{nullptr};
const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
if (n_err != 0 && (ai_hint.ai_flags & AI_ADDRCONFIG) == AI_ADDRCONFIG) {
// AI_ADDRCONFIG on some systems may exclude loopback-only addresse
...

📝 furszy opened a pull request: "assumeUTXO: snapshot load, update VERSION msg height"
(https://github.com/bitcoin/bitcoin/pull/30418)
Due to a missing height update; the node sends its pre-snapshot height to
any peer connecting after the snapshot load but before the node receive
a new tip block.

This does not make any difference for us because we are not using the
`VERSION` msg height field but it does not seem consistent plus other
software might be using the field.

💬 mzumsande commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2218826297)
> However, you forgot GetBlockChecked (and all the other functions that can fail in the same way)?

Updated to change all functions I could find. I was only looking at `getblock` before, not at other RPCs.

>My preference to fix it would be to move the checks to the inside of the block manager.

> However, this basically requires a full rewrite of the blockmanager read-interface, so I haven't completed it yet. (Happy to pick it up again, if you think it makes sense as well)

Makes sense
...

💬 ryanofsky commented on pull request "kernel, logging: Pass Logger instances to kernel objects":
(https://github.com/bitcoin/bitcoin/pull/30342#issuecomment-2218833239)
Rebased db1b9f7696af80036de739408cd9eae5d06481ec -> 42d0e06d1a4343c2bf3257f26cf18b79072c6b0d ([`pr/gklog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/gklog.2) -> [`pr/gklog.3`](https://github.com/ryanofsky/bitcoin/commits/pr/gklog.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gklog.2-rebase..pr/gklog.3)) to fix conflict with #30141. Also added commit to let kernel applications direct low-level util output to custom log instances.

💬 achow101 commented on issue "Send RPC calls performance":
(https://github.com/bitcoin/bitcoin/issues/30416#issuecomment-2218834962)
> #26008 may relate? I am not sure if it was included in 27.0 release

That went into 27.0, and seems like a probable candidate for the performance increase. However, I am a little bit skeptical since it should primarily benefit wallets with a ton of descriptors, and yours only has 24.

Maybe it has a less noticeable benefit for descriptors since the cache is an `std::unsorted_map` while each descriptor has a `std::map` for the scripts.

***

I would have expected that the number of tran
...

💬 achow101 commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2218845486)
> Do you happen to know which pull request added a new positional argument in the middle? Or was this instruction always broken? cc @achow101

Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.



> > A future extension could add an optional return field with device capabilities. Perhaps a descriptor with wildcards. For example: ["pkh("44'/0'/$'/{0,1}/_"), sh(wpkh("49'/0'/$'/{0,1}/_")), wpkh("84'/0'
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2218849345)
Updated 49e0b42e805448423d406edc4e5ebfb34aee959b -> 692fc11f0aae3c8013f6f01d133139ce8eba7135 ([`pr/fatalresult.16`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.16) -> [`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.16..pr/fatalresult.17)) to fix conflict with #30344. Also took suggestions to simplify usage in CompleteChainstateInitialization.

💬 achow101 commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2218850100)
> Hmm, what would happen if `addmultisigaddress` for descriptor wallets just imports the computed descriptor as it does now (with RPC arguments that must be existing addresses or hex pubkeys)? Would it support (participating in) signing for the resulting multisig address (assuming signing for one of specified addresses/pubkeys was possible prior to the RPC call)? Would `listdescriptors true` reveal the corresponding private key?

That breaks current invariants in descriptor wallets that have p
...

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2218998406)
Rebased for silent merge conflict.

💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2219044236)
ACK d93b79470916b1e6f85c55cc6beb1e41b382196f

🚀 achow101 merged a pull request: "tests: improve wallet multisig descriptor test and docs"
(https://github.com/bitcoin/bitcoin/pull/29154)

👍 tdb3 approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2167809049)
ACK 29d19c414c9e138c99f8de84e398528db92ff07e

Left some nits.

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671449310)
nit: Would this be less than or equal to? (equal to in this case).

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671443496)
nit: Would be good to make this comment a `self.log.info()` 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

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671452079)
> ` -TODO: An ancestor of snapshot block`: isn't this already addressed when successfully loading the snapshot into node1 [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_assumeutxo.py#L310)?

nit: Would be good to be more explicit in the log message near the line (or add one more) to document for future readers what the test covers. This way if there are changes in the future, the changes don't accidentally remove case coverage.

🤔 tdb3 reviewed a pull request: "rpc, rest: Improve getblock error when only header is available"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2167870869)
Concept ACK
Thanks for picking this up. Overall, I think adding clarity for the user is a net positive.

> I'm not completely sure if the cause is important enough to change the wording of the rpc error, that some scripts may rely on.

I agree, this may be a borderline case because `Block not found on disk` seems like it is still technically correct albeit not as descriptive. Since we're not changing RPC data structure or error codes (just the message), I'm not even sure if implementing `
...