🤔 stratospher reviewed a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1875752974)
reACK bf5662c6.

📝 instagibbs converted_to_draft a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264)
This allows a transaction's weight to be bound under
a certain weight if possible an desired. This
can be beneficial for future RBF attempts, or
whenever a more restricted spend topology is
desired.

See https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs for more discussion.

Seeking concept ACK and maybe some help on approach for testing.

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#issuecomment-1939199159)
I'm not looking to push this forward for now, if someone wants to take it over be my guest. Draft for now.

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1486576390)
Thanks for testing that @maflcko. I was thinking about this exact possbility when @fjahr suggested his idea a few days ago. I think it 's better to leave node 2 as a test subject.

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1486579322)
Answered on the other thread :)

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1939279337)
> utACK [da13cc2](https://github.com/bitcoin/bitcoin/commit/da13cc2c8010cbccf37324cca4403cb346ecdd30)
>
> I still think my suggestions [here](https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483608953) would be an improvement but are not strictly needed.

Thanks for mentioning that, I think that leaving node 2 was a better alternative becuase it makes this a cleaner and more test , and avoids the errors that might arise in future code changes (mentioned by @maflcko and others that
...

✅ hebasto closed an issue: "When using an unencrypted read-only wallet, pressing "Create Unsigned", shows "This operation needs you wallet passphrase to unlock the wallet""
(https://github.com/bitcoin-core/gui/issues/772)

🚀 hebasto merged a pull request: "Check for private keys disabled before attempting unlock"
(https://github.com/bitcoin-core/gui/pull/773)

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1939302544)
Fixed the last nit: Pushed to remove the extra space mentioned by @fjahr
https://github.com/bitcoin/bitcoin/compare/da13cc2c8010cbccf37324cca4403cb346ecdd30..8d20602e555dfe026b421363ee32cfca17c674d8

💬 knst commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1939304849)
> Seems like adding a const here would be worthwhile. Can you elaborate on the worsened code readability?

here's it: https://github.com/bitcoin/bitcoin/pull/29364/commits/f99d92bde055efaf20c89198eb22dc3e35778d1e
not too bad actually, I added a commit.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486604893)
Removing the spks from the wallet's cache for a particular spkm requires iterating all of the spks and looking for ones that point to this spkm. I think that would be quite slow, and also redundant, since `AddWalletDescriptor` can only ever add scripts to the wallet. Arguably, `UpdateWalletDescriptor` shouldn't be clearing `m_map_script_pub_keys` either.

🤔 BrandonOdiwuor reviewed a pull request: "test, assumeutxo: Add test to ensure failure when mempool not empty"
(https://github.com/bitcoin/bitcoin/pull/29394#pullrequestreview-1875938796)
Concept ACK

🚀 achow101 merged a pull request: "wallet: simplify and batch zap wallet txes process"
(https://github.com/bitcoin/bitcoin/pull/28987)

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486615155)
Good catch, removed.

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1939359268)
reACK https://github.com/bitcoin/bitcoin/pull/26008/commits/a96647308ea40028806e2db6d35fb5dd14bea15f

💬 brunoerg commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1939418351)
> What about setting preferred_net

Yes, I previously discussed this options with @mzumsande. It might be an alternative, sure.

> That would skew the outcome though. If addrman has 60% IPv4, 25% Tor and 15% I2P addresses right now it is more likely to select Tor over I2P. If we run with -onlynet=tor -onlynet=i2p and set preferred_net to Tor or I2P randomly (50% chance) to avoid the IPv4 addresses, then it will not be more likely to pick Tor. It would be relatively easy to extend that parame
...

🤔 furszy reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1876002633)
Left two more comments. They aren't really blocking but I'm still struggling with the memory consumption topic. Isn't `std::unordered_map` going to store all keys (entire scripts) in memory for duplicated so it can solve hash collisions?

Apart from that, just as an extra note for the future: It seems that we will need to rearrange code so that these functions aren't called so many times within the same process.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486652813)
nit: sounds like these two changes should be in the previous commit (cde66ceae).

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486664213)
What is the difference between this (a966473) and the previous commit (f0c844bda) in terms of `CanProvide` usage?

In the previous commit, `CanProvide` usage was removed, while here you kept it.
I understand that you removed the `CanProvide` call from the previous commit because it is redundant (it calls IsMine internally which is exactly what `m_cached_spks` does) but I also agree that we might want to keep `CanProvide` because its internal implementation could vary over time.

💬 mzumsande commented on issue "wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1939541603)
I found the issue by analyzing the logs:
The source of the failure is in an earlier part of the test, at height 205:
There is a race at `node2` between connecting the block produced by `node0`, and using `-generate` to create new blocks itself:
In the failed run, the latter happened first:

[ node2 2024-02-06T11:27:13.136590Z [msghand] [validation.cpp:4015] [AcceptBlockHeader] Saw new header hash=00ed47f85a3d5292b6ed6358f1d1fdee61e54e146fd65ed96a6c9b667edbc9f5 height=205 ](https://cirrus-c
...