💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900961662)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900961662)
Done, thanks
💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900961919)
good idea, pushed.
Thanks
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900961919)
good idea, pushed.
Thanks
💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900964573)
I think this is benching just `blockToJSON` .
But it's a good idea to write a bench for `ReadRawBlockFromDisk` as well.
I will leave that as an idea for a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900964573)
I think this is benching just `blockToJSON` .
But it's a good idea to write a bench for `ReadRawBlockFromDisk` as well.
I will leave that as an idea for a follow-up PR.
💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1900970312)
Yeah, this is currently only a minimum-diff fix, happy to refine further. I assume with "symbol" you mean just another shell script variable, e.g. BUILD_DIR? (Could even go further and allow to specify the build-dir as an optional script parameter that defaults to "build".)
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1900970312)
Yeah, this is currently only a minimum-diff fix, happy to refine further. I assume with "symbol" you mean just another shell script variable, e.g. BUILD_DIR? (Could even go further and allow to specify the build-dir as an optional script parameter that defaults to "build".)
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567933827)
@Sjors
> I still think we should use -blockmaxweight (default 4000000)
Agreed this reflects the current behaviour of this PR
> and then have a separate -maxcoinbaseweight (default 8000). This keeps the same behavior for miners with default settings.
> Miners who manually configured -blockmaxweight before will, after this update, need to manually set -maxcoinbaseweight=4000 to preserve the same behaviour. But if they don't do this, it's not dangerous.
Why do we want to preserve t
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2567933827)
@Sjors
> I still think we should use -blockmaxweight (default 4000000)
Agreed this reflects the current behaviour of this PR
> and then have a separate -maxcoinbaseweight (default 8000). This keeps the same behavior for miners with default settings.
> Miners who manually configured -blockmaxweight before will, after this update, need to manually set -maxcoinbaseweight=4000 to preserve the same behaviour. But if they don't do this, it's not dangerous.
Why do we want to preserve t
...
👍 hodlinator approved a pull request: "depends: Update capnproto to 1.1.0"
(https://github.com/bitcoin/bitcoin/pull/31552#pullrequestreview-2527760917)
cr-ACK b0b8d96d93ea4971e7941ddca03f4393deaac293
Verified hash:
```
₿ curl https://capnproto.org/capnproto-c++-1.1.0.tar.gz -o summed_file
₿ sha256sum summed_file
07167580e563f5e821e3b2af1c238c16ec7181612650c5901330fa9a0da50939 summed_file
```
Cloned repo from GH and ran:
```
₿ git diff v1.0.2 v1.1.0
+
₿ git range-diff v2 v1.0.2 v1.1.0
```
Fairly minor changes. Adding some extra metadata for LSPs, disabling Android CI, some platform specific bugfixes and features, zlib dependency
...
(https://github.com/bitcoin/bitcoin/pull/31552#pullrequestreview-2527760917)
cr-ACK b0b8d96d93ea4971e7941ddca03f4393deaac293
Verified hash:
```
₿ curl https://capnproto.org/capnproto-c++-1.1.0.tar.gz -o summed_file
₿ sha256sum summed_file
07167580e563f5e821e3b2af1c238c16ec7181612650c5901330fa9a0da50939 summed_file
```
Cloned repo from GH and ran:
```
₿ git diff v1.0.2 v1.1.0
+
₿ git range-diff v2 v1.0.2 v1.1.0
```
Fairly minor changes. Adding some extra metadata for LSPs, disabling Android CI, some platform specific bugfixes and features, zlib dependency
...
💬 Prabhat1308 commented on issue "Fuzz: Runtime errors when running fuzz tests on MacOs":
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2567952080)
> Does it work with the `libfuzzer-nosan` preset?
works with the following warnings and without any errors.
```
WARNING: Failed to find function "__sanitizer_acquire_crash_state". Reason dlsym(RTLD_DEFAULT, __sanitizer_acquire_crash_state): symbol not found.
WARNING: Failed to find function "__sanitizer_print_stack_trace". Reason dlsym(RTLD_DEFAULT, __sanitizer_print_stack_trace): symbol not found.
WARNING: Failed to find function "__sanitizer_set_death_callback". Reason dlsym(RTLD_DEFAUL
...
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2567952080)
> Does it work with the `libfuzzer-nosan` preset?
works with the following warnings and without any errors.
```
WARNING: Failed to find function "__sanitizer_acquire_crash_state". Reason dlsym(RTLD_DEFAULT, __sanitizer_acquire_crash_state): symbol not found.
WARNING: Failed to find function "__sanitizer_print_stack_trace". Reason dlsym(RTLD_DEFAULT, __sanitizer_print_stack_trace): symbol not found.
WARNING: Failed to find function "__sanitizer_set_death_callback". Reason dlsym(RTLD_DEFAUL
...
👍 ryanofsky approved a pull request: "test: have miner_tests use Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2527647194)
Code review ACK e4e76f27d73646d887ed8b660c67da4b4b785d8e. Pretty straightforward test updates that add some more coverage. I left some comments below but none are very important.
(https://github.com/bitcoin/bitcoin/pull/31581#pullrequestreview-2527647194)
Code review ACK e4e76f27d73646d887ed8b660c67da4b4b785d8e. Pretty straightforward test updates that add some more coverage. I left some comments below but none are very important.
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900964589)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Can this comment be updated to say the reason for alternating between these? (e.g. to provide test coverage for net_processing code)
I think it would also be clearer to drop the comments about this in the PR and commit descriptions:
- "We mainly call the createNewBlock() method, but also getTip()->height and submitSolution(). Calls to the latter are alternated with direct calls to Chainman's
...
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900964589)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Can this comment be updated to say the reason for alternating between these? (e.g. to provide test coverage for net_processing code)
I think it would also be clearer to drop the comments about this in the PR and commit descriptions:
- "We mainly call the createNewBlock() method, but also getTip()->height and submitSolution(). Calls to the latter are alternated with direct calls to Chainman's
...
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900936095)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
In other tests below this is followed by `BOOST_REQUIRE(miner);`. Seems fine to keep or drop, but would be good to be consistent.
Also I think it might be a little clearer if variable was called `mining` instead of `miner` since it's not a miner and can't mine blocks, it's just a pointer to the interface used for mining. But feel free to ignore this if you prefer the current name.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900936095)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
In other tests below this is followed by `BOOST_REQUIRE(miner);`. Seems fine to keep or drop, but would be good to be consistent.
Also I think it might be a little clearer if variable was called `mining` instead of `miner` since it's not a miner and can't mine blocks, it's just a pointer to the interface used for mining. But feel free to ignore this if you prefer the current name.
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900922458)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Other tests seem to add BOOST_REQUIRE(block_template); before this.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900922458)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Other tests seem to add BOOST_REQUIRE(block_template); before this.
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900923460)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Other tests seem to add BOOST_REQUIRE(block_template); before this.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900923460)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Other tests seem to add BOOST_REQUIRE(block_template); before this.
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900973493)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Unless I'm misunderstanding something it doesn't seem accurate to say that these don't check if the new block is valid. Would seem more accurate to say they return true even if the block is invalid. Also it would seem be more accurate to say that they update the tip, rather than that they wait for the tip to update.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900973493)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Unless I'm misunderstanding something it doesn't seem accurate to say that these don't check if the new block is valid. Would seem more accurate to say they return true even if the block is invalid. Also it would seem be more accurate to say that they update the tip, rather than that they wait for the tip to update.
💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900981503)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Seems fine to call waitTipChange if idea is to add more test coverage for it, but wouldn't it be more direct to just check that the new tip matches the block hash if the idea is to check that the block was connected? This way if the block was not connected the test would fail instead of hanging indefinitely.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900981503)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)
Seems fine to call waitTipChange if idea is to add more test coverage for it, but wouldn't it be more direct to just check that the new tip matches the block hash if the idea is to check that the block was connected? This way if the block was not connected the test would fail instead of hanging indefinitely.
💬 i-am-yuvi commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2567974643)
I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.
clang version:
```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
clang++ version:
```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
and I've used this command mentioned in docs for [macos](https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2567974643)
I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.
clang version:
```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
clang++ version:
```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```
and I've used this command mentioned in docs for [macos](https://github.com/bitcoin/bit
...
🤔 pinheadmz reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2527798109)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2527798109)
concept ACK
💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1901008846)
Why change this? Why not also include nodes 0 & 1?
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1901008846)
Why change this? Why not also include nodes 0 & 1?
💬 maflcko commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901022152)
Shouldn't this be `g_used_system_time |= bool(mocktime.count())`? This would allow to remove the `GetMockTime().count() == 0` check as well.
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901022152)
Shouldn't this be `g_used_system_time |= bool(mocktime.count())`? This would allow to remove the `GetMockTime().count() == 0` check as well.
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901027110)
> Personally, I find the `ByteType auto` construct slightly more confusing than the more verbose template.
Ok, resolving for now.
If there is a preference about the style, my preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901027110)
> Personally, I find the `ByteType auto` construct slightly more confusing than the more verbose template.
Ok, resolving for now.
If there is a preference about the style, my preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.
💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2568022971)
> This change is backward-compatible, as per the old rules, such a signet challenge would always evaluate to false, rendering it unused.
I don't completely agree with this, since starting an old version of bitcoind with (for example)
`-signetchallenge=6a4c0901ff000000000000004c0151` would create an incompatible node. It would have a different network magic bytes and I think the `OP_RETURN` challenge would cause it to reject all blocks.
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2568022971)
> This change is backward-compatible, as per the old rules, such a signet challenge would always evaluate to false, rendering it unused.
I don't completely agree with this, since starting an old version of bitcoind with (for example)
`-signetchallenge=6a4c0901ff000000000000004c0151` would create an incompatible node. It would have a different network magic bytes and I think the `OP_RETURN` challenge would cause it to reject all blocks.