💬 i-am-yuvi commented on pull request "#31403 follow-up":
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573352968)
> ACK [a3f5573](https://github.com/bitcoin/bitcoin/commit/a3f5573ae3aa5edbe92d0388f36586ad02214de6)
>
> First commit follows recommendations by myself with improvements by maflcko in already merged PR.
>
> Passed subset of functional tests locally.
>
> Might squeeze a little bit more information into the title?
>
> "#31403 follow-up" -> "qa: Improve framework.generate* enforcement (#31403 follow-up)"
>
> See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-t
...
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573352968)
> ACK [a3f5573](https://github.com/bitcoin/bitcoin/commit/a3f5573ae3aa5edbe92d0388f36586ad02214de6)
>
> First commit follows recommendations by myself with improvements by maflcko in already merged PR.
>
> Passed subset of functional tests locally.
>
> Might squeeze a little bit more information into the title?
>
> "#31403 follow-up" -> "qa: Improve framework.generate* enforcement (#31403 follow-up)"
>
> See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-t
...
💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573398883)
Thanks for the review @i-am-yuvi!
> I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!
So I learned that it's likely this
https://github.com/bitcoin/bitcoin/blob/41a2ce9b7d7354c633c104bab5653f1f60eb2068/src/test/fuzz/util.cpp#L278-L289
that is preventing the `load_external_block_file` target from crashing on a mac.
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573398883)
Thanks for the review @i-am-yuvi!
> I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!!
So I learned that it's likely this
https://github.com/bitcoin/bitcoin/blob/41a2ce9b7d7354c633c104bab5653f1f60eb2068/src/test/fuzz/util.cpp#L278-L289
that is preventing the `load_external_block_file` target from crashing on a mac.
📝 hebasto opened a pull request: "depends, NetBSD: Fix `bdb` package compilation with GCC-14"
(https://github.com/bitcoin/bitcoin/pull/31613)
On NetBSD 10, compilation of the `bdb` package with GCC-14 fails with the following error:
```
$ gmake -C depends bdb CC=/usr/pkg/gcc14/bin/gcc
<snip>
./libtool --mode=compile /usr/pkg/gcc14/bin/gcc -c -I. -I../dist/./.. -I/home/hebasto/dev/bitcoin/depends/x86_64-unknown-netbsd10.0/include -D_XOPEN_SOURCE=600 -pipe -std=c11 -O2 -Wno-error=implicit-function-declaration -Wno-error=format-security -Wno-error=implicit-int ../dist/./../env/env_config.c
libtool: compile: /usr/pkg/gcc14/bin/g
...
(https://github.com/bitcoin/bitcoin/pull/31613)
On NetBSD 10, compilation of the `bdb` package with GCC-14 fails with the following error:
```
$ gmake -C depends bdb CC=/usr/pkg/gcc14/bin/gcc
<snip>
./libtool --mode=compile /usr/pkg/gcc14/bin/gcc -c -I. -I../dist/./.. -I/home/hebasto/dev/bitcoin/depends/x86_64-unknown-netbsd10.0/include -D_XOPEN_SOURCE=600 -pipe -std=c11 -O2 -Wno-error=implicit-function-declaration -Wno-error=format-security -Wno-error=implicit-int ../dist/./../env/env_config.c
libtool: compile: /usr/pkg/gcc14/bin/g
...
💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573406944)
> I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic
Good catch, thank you. Added in ff21870e20b2391b684cc50fdd6879805055d6a1
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2573406944)
> I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic
Good catch, thank you. Added in ff21870e20b2391b684cc50fdd6879805055d6a1
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904334692)
Ugh, must have switched to the old branch by mistake when I was testing this. I think it would be better to clamp the negative values to zero, which preserves the current behaviour.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1904334692)
Ugh, must have switched to the old branch by mistake when I was testing this. I think it would be better to clamp the negative values to zero, which preserves the current behaviour.
💬 fanquake commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2573424858)
https://cirrus-ci.com/task/5516452708483072:
```bash
[16:01:15.125] cd /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src && /usr/bin/ccache /usr/bin/clang++ -DABORT_ON_FAILED_ASSUME -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DDEBUG -DDEBUG_LOCKCONTENTION -DDEBUG_LOCKORDER -DRPC_DOC_CHECK -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -I/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src -I/ci_container_base/src -isystem /ci_co
...
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2573424858)
https://cirrus-ci.com/task/5516452708483072:
```bash
[16:01:15.125] cd /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src && /usr/bin/ccache /usr/bin/clang++ -DABORT_ON_FAILED_ASSUME -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DDEBUG -DDEBUG_LOCKCONTENTION -DDEBUG_LOCKORDER -DRPC_DOC_CHECK -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -I/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src -I/ci_container_base/src -isystem /ci_co
...
💬 hebasto commented on issue "Building `--with-experimental-kernel-lib` fails on OpenBSD 7.2":
(https://github.com/bitcoin/bitcoin/issues/27242#issuecomment-2573441771)
I believe this issue is no longer relevant after migrating to CMake.
On OpenBSD 7.6, the build completes without any issues:
```
$ cmake -B build -DBUILD_KERNEL_LIB=ON
$ cmake --build build
$ file build/src/kernel/libbitcoinkernel.so
build/src/kernel/libbitcoinkernel.so: ELF 64-bit LSB shared object, x86-64, version 1
```
(https://github.com/bitcoin/bitcoin/issues/27242#issuecomment-2573441771)
I believe this issue is no longer relevant after migrating to CMake.
On OpenBSD 7.6, the build completes without any issues:
```
$ cmake -B build -DBUILD_KERNEL_LIB=ON
$ cmake --build build
$ file build/src/kernel/libbitcoinkernel.so
build/src/kernel/libbitcoinkernel.so: ELF 64-bit LSB shared object, x86-64, version 1
```
💬 maflcko commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2573448664)
Does that mean it needs to be passed again at build time? This seems a bit confusing, given that they are supposed to be vendor settings.
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2573448664)
Does that mean it needs to be passed again at build time? This seems a bit confusing, given that they are supposed to be vendor settings.
🤔 mzumsande reviewed a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2532471105)
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2532471105)
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
💬 mzumsande commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1904342279)
Makes sense, I got confused there!
It just seemed a bit unusual, plus we generally use `CHECK_NONFATAL` instead of `assert` in rpc code, which may not be the ideal choice for possible future interface users (though it is currently just used in another rpc call, so it's not an actual problem).
But I'm not an expert on the design of our interfaces, so I don't know if moving it somewhere else is necessary.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1904342279)
Makes sense, I got confused there!
It just seemed a bit unusual, plus we generally use `CHECK_NONFATAL` instead of `assert` in rpc code, which may not be the ideal choice for possible future interface users (though it is currently just used in another rpc call, so it's not an actual problem).
But I'm not an expert on the design of our interfaces, so I don't know if moving it somewhere else is necessary.
👍 BrandonOdiwuor approved a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2532501177)
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2532501177)
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
👍 ryanofsky approved a pull request: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2532399834)
Code review ACK 5b2d0216d871bd481f733506a2e8f80b8a34eca0. Seems like a nice feature, and the implementation is pretty simple. I think there is a minor regression if scanblocks start is called from multiple threads, and suggested a fix below.
re: https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344018591
> Any ideas on how to slow down `scanblocks()` enough to allow a `status` call to consistently return status of a scan in progress?
Will suggest is a very general solution th
...
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2532399834)
Code review ACK 5b2d0216d871bd481f733506a2e8f80b8a34eca0. Seems like a nice feature, and the implementation is pretty simple. I think there is a minor regression if scanblocks start is called from multiple threads, and suggested a fix below.
re: https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344018591
> Any ideas on how to slow down `scanblocks()` enough to allow a `status` call to consistently return status of a scan in progress?
Will suggest is a very general solution th
...
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904320752)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Think this is missing a word, suggest "ensure this is _called_ before"
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904320752)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Think this is missing a word, suggest "ensure this is _called_ before"
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904339347)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Lock order here seems a little subtle. Would be good to have a comment saying it is important to lock this before calling reserve so if a scan is currently happening but is about to finish, an accurate "relevant_blocks" value can be returned before the scanning thread clears it.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904339347)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Lock order here seems a little subtle. Would be good to have a comment saying it is important to lock this before calling reserve so if a scan is currently happening but is about to finish, an accurate "relevant_blocks" value can be returned before the scanning thread clears it.
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904332839)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Clearing `relevant_blocks` before checking `g_scanfilter_in_progress` seems not very nice, because it means if scanblocks start is called while another scan is in progress, instead of just returning an "already in progress" error, it will do that but also partially erase the state of the in-progress scan.
This behavior is not just a limitation of the new feature, but also a regression, beca
...
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904332839)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Clearing `relevant_blocks` before checking `g_scanfilter_in_progress` seems not very nice, because it means if scanblocks start is called while another scan is in progress, instead of just returning an "already in progress" error, it will do that but also partially erase the state of the in-progress scan.
This behavior is not just a limitation of the new feature, but also a regression, beca
...
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904300535)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
It would be good to declare these next to `g_scanfilter_` variables so all global state is declared in one place, and so other functions besides `scanblocks` can see this information (other RPCs and maybe tests should be able to access it). Would suggest renaming `relevant_blocks` to `g_scanfilter_relevant_blocks` and `cs_relevant_blocks` to `g_scanfilter_relevant_blocks_mutex`. (The "critical
...
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904300535)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
It would be good to declare these next to `g_scanfilter_` variables so all global state is declared in one place, and so other functions besides `scanblocks` can see this information (other RPCs and maybe tests should be able to access it). Would suggest renaming `relevant_blocks` to `g_scanfilter_relevant_blocks` and `cs_relevant_blocks` to `g_scanfilter_relevant_blocks_mutex`. (The "critical
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2573467782)
Updated ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 -> 54bdaaefbc683a61f3051dd90bbaf69de5d6cac5 ([kernel_cache_sizes_5](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_5) -> [kernel_cache_sizes_6](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_5..kernel_cache_sizes_6))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1903999989), took sugg
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2573467782)
Updated ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 -> 54bdaaefbc683a61f3051dd90bbaf69de5d6cac5 ([kernel_cache_sizes_5](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_5) -> [kernel_cache_sizes_6](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_5..kernel_cache_sizes_6))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1903999989), took sugg
...
💬 i-am-yuvi commented on pull request "qa: Improve framework.generate* enforcement (#31403 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573527165)
> Also, a rebase would be good
Done!
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573527165)
> Also, a rebase would be good
Done!
💬 i-am-yuvi commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2573560025)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2573560025)
Concept ACK
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2573570012)
Rebased on the latest #31581.
I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.
Since `miner_tests.cpp` uses mainnet, I created `testnet4_miner_tests.cpp` to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583.
I also switched to using `NodeClock::now()` to be able to use mock time in tests.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2573570012)
Rebased on the latest #31581.
I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.
Since `miner_tests.cpp` uses mainnet, I created `testnet4_miner_tests.cpp` to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583.
I also switched to using `NodeClock::now()` to be able to use mock time in tests.