💬 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.
(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
(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
...
(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.
(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).
(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.
(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
...
(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
(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
...
(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
...
(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
...
🤔 furszy reviewed a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1876349250)
ACK https://github.com/bitcoin/bitcoin/commit/f1684bb88a878eb3f54e945db39ea69b14256eef
> The only two outstanding performance improvements are https://github.com/bitcoin/bitcoin/pull/28987 and https://github.com/bitcoin/bitcoin/pull/26008
There are actually more. #28987, and also #29403, are prerequisites for further performance improvements on #28574.
That being said, while I would like to reach that goal (same as getting closer in #26008), I also think that we should move forward with
...
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1876349250)
ACK https://github.com/bitcoin/bitcoin/commit/f1684bb88a878eb3f54e945db39ea69b14256eef
> The only two outstanding performance improvements are https://github.com/bitcoin/bitcoin/pull/28987 and https://github.com/bitcoin/bitcoin/pull/26008
There are actually more. #28987, and also #29403, are prerequisites for further performance improvements on #28574.
That being said, while I would like to reach that goal (same as getting closer in #26008), I also think that we should move forward with
...
💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939619126)
> Rebased. This has failed CI once when waiting for the disconnection after:
>
> ```
> cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
> ```
>
> I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead.
>
> PS: For context, you can check the failure here: https://cirrus-ci.com/task/4624744360706048?logs=ci#L6520. See how the peer is giv
...
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939619126)
> Rebased. This has failed CI once when waiting for the disconnection after:
>
> ```
> cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
> ```
>
> I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead.
>
> PS: For context, you can check the failure here: https://cirrus-ci.com/task/4624744360706048?logs=ci#L6520. See how the peer is giv
...
💬 hebasto commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939630213)
> This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?
Last [time](https://github.com/bitcoin-core/gui/issues/112) I tried to replace `NSUserNotificationCenter` with `UNUserNotificationCenter` on macOS 10.15, I got the authorization error "Notifications are not allowed for this application".
Now, the minimum supported macOS is 11.0. So it might be worth trying once more.
> > It is Qt 5 code base. I did not check it out, but I hope Qt
...
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939630213)
> This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?
Last [time](https://github.com/bitcoin-core/gui/issues/112) I tried to replace `NSUserNotificationCenter` with `UNUserNotificationCenter` on macOS 10.15, I got the authorization error "Notifications are not allowed for this application".
Now, the minimum supported macOS is 11.0. So it might be worth trying once more.
> > It is Qt 5 code base. I did not check it out, but I hope Qt
...
💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1939644589)
> @dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I'll update it.
Sorry for the back-and-forth, but I confused myself here. `--disable-asm` doesn't have any effect on libsecp, but `--without-asm` does. No need to update the PR description.
>
> It still forwards just fine with our option removed though, so I don't think that changes anything here.
I got this wrong but it still accide
...
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1939644589)
> @dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I'll update it.
Sorry for the back-and-forth, but I confused myself here. `--disable-asm` doesn't have any effect on libsecp, but `--without-asm` does. No need to update the PR description.
>
> It still forwards just fine with our option removed though, so I don't think that changes anything here.
I got this wrong but it still accide
...
💬 fanquake commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939655020)
I guess mark as draft or something else for now then? This can't be merged as-is, because the comments in the config of files are not correct.
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939655020)
I guess mark as draft or something else for now then? This can't be merged as-is, because the comments in the config of files are not correct.
💬 hebasto commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939662337)
> the comments in the config of files are not correct.
Comments have been adjusted.
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1939662337)
> the comments in the config of files are not correct.
Comments have been adjusted.
💬 vostrnad commented on issue "Assertion chainman().ActiveChain()[height] fails on startup on memory-constrained system":
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-1939703478)
I can reproduce the issue every time on my machine. Here it is with `-debug=1`:
<details>
<summary>Logs before being killed</summary>
```
2024-02-12T20:01:01Z [net] Requesting block 000000000000073bc48e2c26389c0034f0c3fc448cfc0571fc0382bce8f3aa0d (141081) peer=25
2024-02-12T20:01:01Z [net] sending getdata (37 bytes) peer=25
2024-02-12T20:01:01Z [net] received: block (25657 bytes) peer=18
2024-02-12T20:01:01Z [net] received block 000000000000045d8ee15145b337a68a8fcac0ec3383a6dc66fe8231
...
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-1939703478)
I can reproduce the issue every time on my machine. Here it is with `-debug=1`:
<details>
<summary>Logs before being killed</summary>
```
2024-02-12T20:01:01Z [net] Requesting block 000000000000073bc48e2c26389c0034f0c3fc448cfc0571fc0382bce8f3aa0d (141081) peer=25
2024-02-12T20:01:01Z [net] sending getdata (37 bytes) peer=25
2024-02-12T20:01:01Z [net] received: block (25657 bytes) peer=18
2024-02-12T20:01:01Z [net] received block 000000000000045d8ee15145b337a68a8fcac0ec3383a6dc66fe8231
...
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486898612)
I think this may have just been a holdover from when I was investigating using the same cache for signing. There should be no difference as `CanProvide` is redundant in both contexts.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486898612)
I think this may have just been a holdover from when I was investigating using the same cache for signing. There should be no difference as `CanProvide` is redundant in both contexts.
💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1939755941)
> The one difference I notice without `--disable-asm` is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):
>
@jonatack Since you can reproduce, any interest in testing this theory https://github.com/bitcoin-core/crc32c-subtree/pull/6#issuecomment-1936095430 ?
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1939755941)
> The one difference I notice without `--disable-asm` is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):
>
@jonatack Since you can reproduce, any interest in testing this theory https://github.com/bitcoin-core/crc32c-subtree/pull/6#issuecomment-1936095430 ?
💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939768407)
> There doesn't seem to be any sane way to make this a scripted diff
@TheCharlatan saw this and said "hold my beer". Amazing!
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939768407)
> There doesn't seem to be any sane way to make this a scripted diff
@TheCharlatan saw this and said "hold my beer". Amazing!