💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486449111)
Thank you, yes, I should have been clearer. The updated `test_outbound_eviction_protected_mixed` in 91005f7459993e1f7139e46b53eef1bb04860a7c looks great (includes 4 protected, 4 unprotected (2 honest, 2 misbehaving by sending no headers or old)). ACK

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486473051)
We'd have to make some kind of decision what to default to - maybe v1 now and switch to v2 after a majority of the network has upgraded?
My idea for now was to only add `--v1transport` in `test_runner.py` the few tests that are run twice, not everywhere.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486494438)
Shouldn't be a problem but, just to keep this process consistent; as `UpdateWalletDescriptor` clears the SPKM internal scripts cache, shouldn't do the same for the general wallet cache too?
E.g. remove the scripts belonging to this spkm here, so the `TopUp` call that is executed few lines below (re)populates the scripts derived from the updated SPKM.

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