💬 achow101 commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1946144606)
Legacy wallet removal #20160

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1491051732)
Well it's not random, it's a fuzzer input. They're random in a way, but definitely not uniformly random.

I think the sorting is there to make sure that just reorderings in the list don't affect fuzzer behavior.

💬 TheCharlatan commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1946153756)
libbitcoinkernel #27587

💬 ismaelsadeeq commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1491054596)
Yes thanks for clarification 👍🏾

💬 josibake commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1946158788)
Silent payments (BIP352) #28536

💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#issuecomment-1946159253)
Light utACK 0a3b5739ce8b9a2d219bcf3208069436c66fd5bb

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1491058952)
sorry I got confused by the comment and thought you meant that I changed the log line to include `disk` first and then in the second commit changed it to `file`

I updated `LogPrintf` -> `LogInfo` in [27251e8](https://github.com/bitcoin/bitcoin/pull/29402/commits/27251e89b67f83a40cfd70eeded4120a28f82cf9)

💬 ismaelsadeeq commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1491060699)
I looked at it the other way around, using the result from `BuildDiagramFromUnsortedChunks` sort which custom > operator @instagibbs.

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1491062613)
instead, I opted not to cast the value and instead print out the number of bytes, let me know if MB is strongly preferred and I can find a solution.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1946168171)
Added `m_tp_mutex` to `Sv2TemplateProvider`.

💬 fjahr commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1946170816)
Default ASMap #28794

There isn't so much left to do in terms of code/review within the repo but it needs some more focus from reviewers to finish it up. Until recently I considered file creation creation and tooling outside of Bitcoin Core a blocker, which I why I didn't propose it for v27. Now I think we got that out of the way with a simple process that has been demonstrated to work with many contributors involved: https://delvingbitcoin.org/t/asmap-creation-process/548

💬 achow101 commented on issue "wallet: pre-HD HDD migratewallet ":
(https://github.com/bitcoin/bitcoin/issues/29438#issuecomment-1946184974)
It seems like most of the time is being spent in reading the wallet rather than writing it?

Can you provide the raw perf data?

💬 furszy commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1946189748)
Prune node rescan #29183.

Mid-size functionality. Mentioning in response to requests from users and contributors.

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1491090655)
@sdaftuar Indeed. It wouldn't require anything more than dropping those `Assume()` calls, but right now FeeFrac objects represent (the aggregate fee and size of) sets of transactions, rather differences between such aggregates (as would be needed for this use case).

💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1946203298)
It would be interesting where this changes the code. I presume the places are:

* The `timingsafe_bcmp` impl in the code was previously always picked. Now it may or may not be picked.
* `LogQtInfo` will always print "dynamic" for plugin. Now it will print the correct thing.



Also, there is an iwyu false-positive in the CI output:

```
httpserver.cpp should remove these lines:
- #include <config/bitcoin-config.h> // lines 6-6

💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1946203944)
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3f 🚪

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 9d1dbbd4ceb8c04340927f51271
...

👍 ryanofsky approved a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1882776315)
Code review ACK b20d882fd53c21098eb7858b2dbca84eafd2b344. I don't have much insight into over performance impact of this change, but it seems like it should make migrated legacy wallets much faster (10 times faster loading in one case: https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180) even if it does increase memory usage, and and it seems to be implemented correctly.

re: https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1876002633

> Apart from that, jus
...

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491049788)
In commit "wallet: Cache scriptPubKeys for all DescriptorSPKMs" (75f0fcf2ef17e3844dac6445913f31fecc4f76b1)

It would be helpful to have a comment here explaining why this is necessary. I assume this is related to the "Use no-op topup_callback, cache is filled later in migration" comments in the earlier commit, but I don't understand why migration code needs this special flow instead of using topup_callback / CacheNewScriptPubKeys like other wallet code. So this would be a good place for a comm
...

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491078659)
In commit "wallet: Use scriptPubKey cache in GetScriptPubKeyMans" (38cfcf3435a5113606c8066833d49a4ab560aea9)

New code is no longer calling CanProvide. Would be good to use Assert or Assume to document that it should be true and verify integrity of the cache. (Same comment applies to GetSolvingProvider in next commit too)

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491016791)
In commit "wallet: Introduce a callback called after TopUp completes" (e06705140d09adb9baf83e24babc575ecdfc0f38)

Passing both WalletStorage callbacks and a separate topup_callback to DescriptorScriptPubKeyMan objects seems awkward. Is there a reason topup_callback is not just a WalletStorage method?