💬 vasild commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486432039)
Alright, "it's the same on master" that is a good point!

So, if we have `--v1transport` (for each individual test and for `test_runner.py`?) then what would happen if ran without either one of `--v1transport` or `--v2transport`?

💬 LarryRuane commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1939061997)
> Is there a better UX option here? What about `bitcoin-cli helpdetail`?

I agree your suggestion would be better, or maybe even `bitcoin-cli -helpdetail` (with a dash) because as it is now, "detail" seems like the name of an RPC. I'll look into what it would take to do either of those, thanks for the suggestion.

👍 vasild approved a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781#pullrequestreview-1875650020)
ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939071798)
I redefined how peers are split in `p2p_eviction:test_outbound_eviction_protected_mixed` to address https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485375517. Both commits can be squashed (leaving them as they are for now in case someone does not agree with the approach)

💬 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)