💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1671260720)
Done, thanks!

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1671261223)
I don't think it will add anything to have a test for that, no.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1671261364)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2218763464)
@paplorinc thank you again for your review! I addressed all your nits I think. I don't think that benchmark is related, could just be noise.

💬 achow101 commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2218778250)
ACK 6ecda04fefad980872c72fba89844393f5581120

💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2218790880)
Rebased 179a0715cdcb3f20f71bf757c37046b0675dc8a5 -> d34b529ed104a3a7e11c7d264703e61d84b1aeb3 ([`pr/alert.1`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.1) -> [`pr/alert.2`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/alert.1-rebase..pr/alert.2)) due to silent conflict with #29625

🚀 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