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

📝 mzumsande opened a pull request: "test: fix intermittent failure in wallet_reorgrestore.py"
(https://github.com/bitcoin/bitcoin/pull/29425)
By adding a missing `sync_blocks` call.
There was a race at `node2` between connecting the block produced by `node0`, and using `-generate` to create new blocks itself. In the failed run, block generation started before connecting the block, resulting in a final block height that was smaller by 1 than expected.
See https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1939541603 for a more detailed analysis of the failed run.

Fixes #29392

🤔 tdb3 reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1876248880)
ACK for 6dddc1c2eda514d35219cce03c229bd6f822be55.
Great work on a great feature. Seems to work well. Reviewed the code changes, but didn't see anything noteworthy over what vasild, luke-jr, and willcl-ark already saw.

Ran the following test actions:
- Cloned and checked out PR branch, compiled, and ran all functional tests (all passed)
- Configured Tor to listen on only the unix domain socket /run/tor/socks (SocksPort 9050 and ControlPort 9051 disabled)
- Started bitcoind (signet), `pro
...

💬 Crypt-iQ commented on issue "Seek more/different peers when ours all have too high feefilter":
(https://github.com/bitcoin/bitcoin/issues/28371#issuecomment-1939604817)
> Maybe it would be good to collect some empirical data of how frequent peers with significantly higher minfeefilter actually are.

I made a network crawler for `feefilter` (sends version/verack and waits for feefilter) and for today this is the histogram of feefilters (sat/KvB) from ~16.8k peers:


Right now, mempool.space says that the minimum fee to get into the mempool is at 4.3
...