👍 ryanofsky approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2454619280)
Code review ACK 027ce3e9634b2f47c508899942d05690572de516. Since last review, this was rebased to avoid conflicts, and option is now a plain cscript instead of an optional\<cscript>, and the commits are split up and there are some minor changes in tests (renames, whitespace, dropping redundant assignments. This is pretty easy to review now that the main commit is so small and changes are split up
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2454619280)
Code review ACK 027ce3e9634b2f47c508899942d05690572de516. Since last review, this was rebased to avoid conflicts, and option is now a plain cscript instead of an optional\<cscript>, and the commits are split up and there are some minor changes in tests (renames, whitespace, dropping redundant assignments. This is pretty easy to review now that the main commit is so small and changes are split up
💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1854018677)
Thanks, cleared the machine
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1854018677)
Thanks, cleared the machine
💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493919528)
I dropped the if guard.
> ensuring that any interfaces will only be spun up after that point in init
It could, but I'm not sure if it's worth putting such a restriction on the interfaces.
Similarly we have `startupnotify` which happens at the end of init. If we also ensure that the interfaces are ready to listen _before_ this notification, then at least from an application launched with systemd (or similiar) we could expect them to wait for this signal before trying to connect.
If on
...
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493919528)
I dropped the if guard.
> ensuring that any interfaces will only be spun up after that point in init
It could, but I'm not sure if it's worth putting such a restriction on the interfaces.
Similarly we have `startupnotify` which happens at the end of init. If we also ensure that the interfaces are ready to listen _before_ this notification, then at least from an application launched with systemd (or similiar) we could expect them to wait for this signal before trying to connect.
If on
...
👍 stickies-v approved a pull request: "refactor: Clamp worker threads in ChainstateManager constructor"
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2454240562)
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
---
I noticed this issue while reviewing #30595, and started working on an [alternative approach](https://github.com/stickies-v/bitcoin/commits/2024-11/add-bounded-integer/) before @TheCharlatan made me aware of this PR. I thought it would be nice to introduce a new `BoundedInt` type, that avoids relying on input validation and instead forces the underlying value to be bound to a (compile time constant) certain range.
However, there were much
...
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2454240562)
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
---
I noticed this issue while reviewing #30595, and started working on an [alternative approach](https://github.com/stickies-v/bitcoin/commits/2024-11/add-bounded-integer/) before @TheCharlatan made me aware of this PR. I thought it would be nice to introduce a new `BoundedInt` type, that avoids relying on input validation and instead forces the underlying value to be bound to a (compile time constant) certain range.
However, there were much
...
💬 stickies-v commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1853795666)
nit: `CCheckQueue` is used for other things than `ScriptCheck` (in test code), but I don't see a better place to put the log statement, and making it a non-refactor commit by e..g updating the wording to "Verification queue uses ..." is probably not worth it either, so disregarding this nit might be the best way forward.
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1853795666)
nit: `CCheckQueue` is used for other things than `ScriptCheck` (in test code), but I don't see a better place to put the log statement, and making it a non-refactor commit by e..g updating the wording to "Verification queue uses ..." is probably not worth it either, so disregarding this nit might be the best way forward.
💬 stickies-v commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1853999686)
Related (but not required for this PR, and currently a no-op), the below patch removing default value duplication would ensure the default `work_threads_num` value doesn't depend on argsmanager being used either:
<details>
<summary>git diff on 8f85d36d68</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 94a5a08463..61c6f060f1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -32,6 +32,7 @@
#include <interfaces/ipc.h>
#include <interfaces/mining.h>
#include <interfa
...
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1853999686)
Related (but not required for this PR, and currently a no-op), the below patch removing default value duplication would ensure the default `work_threads_num` value doesn't depend on argsmanager being used either:
<details>
<summary>git diff on 8f85d36d68</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 94a5a08463..61c6f060f1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -32,6 +32,7 @@
#include <interfaces/ipc.h>
#include <interfaces/mining.h>
#include <interfa
...
💬 ryanofsky commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493956840)
> It could, but I'm not sure if it's worth putting such a restriction on the interfaces.
Tend to agree for now. Having waitTipChanged wait for a tip to be connected seems like a natural thing to implement and might avoid the need for clients to poll the node on startup. But I think this check could be dropped in BlockTemplate::waitNext in #31283 (maybe replaced with an assert there) since it should not be possible to get a blocktemplate reference before the tip is connected. After #31283 it m
...
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493956840)
> It could, but I'm not sure if it's worth putting such a restriction on the interfaces.
Tend to agree for now. Having waitTipChanged wait for a tip to be connected seems like a natural thing to implement and might avoid the need for clients to poll the node on startup. But I think this check could be dropped in BlockTemplate::waitNext in #31283 (maybe replaced with an assert there) since it should not be possible to get a blocktemplate reference before the tip is connected. After #31283 it m
...
💬 maflcko commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493984478)
> source based code coverage using Clang currently still works fine with master (see: [#31337 (comment)](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)).
Would be nice to do it for this pull as well, to see if there is any difference.
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493984478)
> source based code coverage using Clang currently still works fine with master (see: [#31337 (comment)](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)).
Would be nice to do it for this pull as well, to see if there is any difference.
🤔 BrandonOdiwuor reviewed a pull request: "Make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2454713890)
Concept ACK transitioning `m_tip_block` to `std::optional` which remains unset until a block is connected
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2454713890)
Concept ACK transitioning `m_tip_block` to `std::optional` which remains unset until a block is connected
💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2493991348)
My Guix build:
```
aarch64
606d0909c4591fc7dac3759e230e7bd3de00555c1c535d437ca8bc19df85fc70 guix-build-ee1128ead846/output/aarch64-linux-gnu/SHA256SUMS.part
4aca1c476b6824d485044c6636ce2ecf45f542a89bc501493f4868cf16c13d1f guix-build-ee1128ead846/output/aarch64-linux-gnu/bitcoin-ee1128ead846-aarch64-linux-gnu-debug.tar.gz
46d17a50226b60af12124b9c2b70cdab1c257a2034463999c0340d8a8b573cdd guix-build-ee1128ead846/output/aarch64-linux-gnu/bitcoin-ee1128ead846-aarch64-linux-gnu.tar.gz
d82c381c
...
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2493991348)
My Guix build:
```
aarch64
606d0909c4591fc7dac3759e230e7bd3de00555c1c535d437ca8bc19df85fc70 guix-build-ee1128ead846/output/aarch64-linux-gnu/SHA256SUMS.part
4aca1c476b6824d485044c6636ce2ecf45f542a89bc501493f4868cf16c13d1f guix-build-ee1128ead846/output/aarch64-linux-gnu/bitcoin-ee1128ead846-aarch64-linux-gnu-debug.tar.gz
46d17a50226b60af12124b9c2b70cdab1c257a2034463999c0340d8a8b573cdd guix-build-ee1128ead846/output/aarch64-linux-gnu/bitcoin-ee1128ead846-aarch64-linux-gnu.tar.gz
d82c381c
...
💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493995709)
> > source based code coverage using Clang currently still works fine with master (see: [#31337 (comment)](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)).
>
> Would be nice to do it for this pull as well, to see if there is any difference.
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493995709)
> > source based code coverage using Clang currently still works fine with master (see: [#31337 (comment)](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)).
>
> Would be nice to do it for this pull as well, to see if there is any difference.
cc @dergoegge
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854105033)
Apologies l0rinc. I think we are probably just misunderstanding each other. All the checks you are suggesting are possible with the approach I'm suggesting, and it should only make them easier to implement and understand. And I might not understand what approach you prefer since your first comment https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850778259 seems to want new tests to be in the same commit as the bugfix, but the last one https://github.com/bitcoin/bitcoin/pull/31212#discu
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854105033)
Apologies l0rinc. I think we are probably just misunderstanding each other. All the checks you are suggesting are possible with the approach I'm suggesting, and it should only make them easier to implement and understand. And I might not understand what approach you prefer since your first comment https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850778259 seems to want new tests to be in the same commit as the bugfix, but the last one https://github.com/bitcoin/bitcoin/pull/31212#discu
...
💬 davidgumberg commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2494069961)
ACK https://github.com/bitcoin/bitcoin/commit/ee1128ead846698db5e5633f193883837f2fbc64
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2494069961)
ACK https://github.com/bitcoin/bitcoin/commit/ee1128ead846698db5e5633f193883837f2fbc64
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1854157411)
Thank you!
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1854157411)
Thank you!
💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494100649)
Several reindex related tests are broken, investigating...
> After https://github.com/bitcoin/bitcoin/pull/31283 it might make sense to drop the `waitTipChanged` method entirely too.
I think it's generically useful though. There are various types of applications that need to know when a new block is added to the chain.
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494100649)
Several reindex related tests are broken, investigating...
> After https://github.com/bitcoin/bitcoin/pull/31283 it might make sense to drop the `waitTipChanged` method entirely too.
I think it's generically useful though. There are various types of applications that need to know when a new block is added to the chain.
👍 instagibbs approved a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2455019300)
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
Not an expert here but motivation makes a lot of sense, and good to remove such indirect code from past covert fixes
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2455019300)
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
Not an expert here but motivation makes a lot of sense, and good to remove such indirect code from past covert fixes
💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494237583)
Specifically `-reindex-chainstate` is broken, easy to reproduce with e.g. testnet4.
It stops after the second block:
```
2024-11-22T16:44:14.338986Z Setting NODE_NETWORK on non-prune mode
2024-11-22T16:44:14.339014Z Wait for genesis block to be processed
2024-11-22T16:44:14.339023Z initload thread start
2024-11-22T16:44:14.341109Z UpdateTip: new best=00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043 height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2024-05-03T23
...
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2494237583)
Specifically `-reindex-chainstate` is broken, easy to reproduce with e.g. testnet4.
It stops after the second block:
```
2024-11-22T16:44:14.338986Z Setting NODE_NETWORK on non-prune mode
2024-11-22T16:44:14.339014Z Wait for genesis block to be processed
2024-11-22T16:44:14.339023Z initload thread start
2024-11-22T16:44:14.341109Z UpdateTip: new best=00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043 height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2024-05-03T23
...
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494293452)
@glozow I think I agree your branch at 4737df3f6512d2d9c7f8aa95e0635b9d03031402 is superior to this.
> Considering AcceptSubPackage is already smoothing out those joints, I like the idea of getting rid of ATMP in the future, just throwing things at ProcessNewPackage, and enumerating what we can handle at the start of AcceptPackage.
Yes, I would love to get rid of the separate paths over time.
> 've also implemented a removal of the is-child-with-unconfirmed-parents rule on top of it,
...
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494293452)
@glozow I think I agree your branch at 4737df3f6512d2d9c7f8aa95e0635b9d03031402 is superior to this.
> Considering AcceptSubPackage is already smoothing out those joints, I like the idea of getting rid of ATMP in the future, just throwing things at ProcessNewPackage, and enumerating what we can handle at the start of AcceptPackage.
Yes, I would love to get rid of the separate paths over time.
> 've also implemented a removal of the is-child-with-unconfirmed-parents rule on top of it,
...
📝 yancyribbens opened a pull request: "Add coin-grinder example test"
(https://github.com/bitcoin/bitcoin/pull/31352)
In understanding the coin-grinder algorithm, I find it useful to run the algorithm with the parameters given in the example code. I thing it would be useful to add to the test-framework.
(https://github.com/bitcoin/bitcoin/pull/31352)
In understanding the coin-grinder algorithm, I find it useful to run the algorithm with the parameters given in the example code. I thing it would be useful to add to the test-framework.
💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494305256)
thanks for looking!
> I'll have to do a bunch more thinking and clearly requires its own PR.
>
In case it wasn’t clear, I definitely meant that as a separate PR. I only
implemented it to sketch out what it’d look like as the next step.
>
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494305256)
thanks for looking!
> I'll have to do a bunch more thinking and clearly requires its own PR.
>
In case it wasn’t clear, I definitely meant that as a separate PR. I only
implemented it to sketch out what it’d look like as the next step.
>