📝 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!
💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1939776161)
reACK 77331aa2a198b708372a5c6cdf331faf7e2968ef
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1939776161)
reACK 77331aa2a198b708372a5c6cdf331faf7e2968ef
💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003)
Replaced my commit with @TheCharlatan's artwork above.
It doesn't work in the c-i environment because `bitcoin-config.h.in` is missing. I added an `./autogen.sh` to the script but that doesn't help because that env doesn't have autoconf.
Next I tried prepending with a bare `autoheader`, which should be all that's actually needed, but that's missing too.
Sadly, I'm not sure it's worth continuing with this. Even if it does work, the verify script would have to modify the user's tree which
...
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003)
Replaced my commit with @TheCharlatan's artwork above.
It doesn't work in the c-i environment because `bitcoin-config.h.in` is missing. I added an `./autogen.sh` to the script but that doesn't help because that env doesn't have autoconf.
Next I tried prepending with a bare `autoheader`, which should be all that's actually needed, but that's missing too.
Sadly, I'm not sure it's worth continuing with this. Even if it does work, the verify script would have to modify the user's tree which
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939902686)
> > 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
...
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939902686)
> > 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
...
💬 TheCharlatan commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1940697823)
Re https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003
Could just replace the first command then. Not as nice, but for the purpose of this PR, reviewers can run the mentioned command manually: https://github.com/TheCharlatan/bitcoin/commit/b78e3ce41168600a1e73cb079b7eba07893753c4 (also applies some fixes, because the verify script uses `sh` and not `bash` :P).
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1940697823)
Re https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003
Could just replace the first command then. Not as nice, but for the purpose of this PR, reviewers can run the mentioned command manually: https://github.com/TheCharlatan/bitcoin/commit/b78e3ce41168600a1e73cb079b7eba07893753c4 (also applies some fixes, because the verify script uses `sh` and not `bash` :P).
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1487390188)
Unless there's an objection, I'd follow up with this in a follow-up?
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1487390188)
Unless there's an objection, I'd follow up with this in a follow-up?
💬 maflcko commented on pull request "getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1940767749)
Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?
If this is merged, then every RPC will be duplicated in a new GUI pop-up?
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1940767749)
Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?
If this is merged, then every RPC will be duplicated in a new GUI pop-up?
💬 maflcko commented on issue "Bitcoin Core GUI getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/issues/645#issuecomment-1940770556)
Not sure. Seems easier to just use the RPC window, with the benefit that it has more docs and doesn't need an update if the RPC changes.
(https://github.com/bitcoin-core/gui/issues/645#issuecomment-1940770556)
Not sure. Seems easier to just use the RPC window, with the benefit that it has more docs and doesn't need an update if the RPC changes.