💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580767132)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?
`uint8_t` and `unsigned char` are the exact same type. If not, the compilation would fail in other places. I don't think it is helpful to create 2432 pull requests to change each instance one-by-one in a separate pull request. If this change i
...
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580767132)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?
`uint8_t` and `unsigned char` are the exact same type. If not, the compilation would fail in other places. I don't think it is helpful to create 2432 pull requests to change each instance one-by-one in a separate pull request. If this change i
...
💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580783610)
> As I said in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742 this seems like an optimization that can wait.
Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580783610)
> As I said in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742 this seems like an optimization that can wait.
Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.
👍 ryanofsky approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2540386980)
Code review ACK 384c73d4fcda4af7c2b4bfb945a248cb93dba47d but it seems like a fuzz check is failing in CI. Since last review, BlockManagerOpts uses DBParams, so no longer needs to duplicate most of its fields, and blockstorage m_opts no longer need to be public so some other changes could be reverted. One other set of changes that were reverted had do to with `m_block_tree_db_in_memory` / `opts.block_tree_db_in_memory;` which seems more correct now.
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2540386980)
Code review ACK 384c73d4fcda4af7c2b4bfb945a248cb93dba47d but it seems like a fuzz check is failing in CI. Since last review, BlockManagerOpts uses DBParams, so no longer needs to duplicate most of its fields, and blockstorage m_opts no longer need to be public so some other changes could be reverted. One other set of changes that were reverted had do to with `m_block_tree_db_in_memory` / `opts.block_tree_db_in_memory;` which seems more correct now.
💬 ryanofsky commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909120127)
In commit "refactor: Remove redundant reindex check" (fd590d5ca1cfb6c2525b7755edb6a3a2cf16c856)
Curious about something: In https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2397922175 you wrote,
> Yes, my eventual goal was to move calling `InitializeChainstate` for the fully validated chainstate into the `ChainstateManager` constructor as well as calling `InitCoinsDB` for it. I wanted to do this in a future pull request if this change is accepted.
So if coins db were intialize
...
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909120127)
In commit "refactor: Remove redundant reindex check" (fd590d5ca1cfb6c2525b7755edb6a3a2cf16c856)
Curious about something: In https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2397922175 you wrote,
> Yes, my eventual goal was to move calling `InitializeChainstate` for the fully validated chainstate into the `ChainstateManager` constructor as well as calling `InitCoinsDB` for it. I wanted to do this in a future pull request if this change is accepted.
So if coins db were intialize
...
💬 ryanofsky commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909124834)
In commit "kernel: Move block tree db open to block manager" (384c73d4fcda4af7c2b4bfb945a248cb93dba47d)
Looks like indentation unintentionally changed here
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909124834)
In commit "kernel: Move block tree db open to block manager" (384c73d4fcda4af7c2b4bfb945a248cb93dba47d)
Looks like indentation unintentionally changed here
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909147382)
Taken, although I moved some of the handling of `expected_ret_code` directly into `is_node_stopped()`.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909147382)
Taken, although I moved some of the handling of `expected_ret_code` directly into `is_node_stopped()`.
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909144554)
> I mostly found the previous comments easier to follow because they were more direct and interspersed with relevant code.
Hopefully this is from the latest push can be considered an improvement:
```python
if e.errno not in [ errno.ECONNRESET, # This might happen when the RPC server is in warmup,
# but shut down before the call to getblockcount succeeds.
errno.ETIMEDOUT, # Treat identical to ECONNRESET
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909144554)
> I mostly found the previous comments easier to follow because they were more direct and interspersed with relevant code.
Hopefully this is from the latest push can be considered an improvement:
```python
if e.errno not in [ errno.ECONNRESET, # This might happen when the RPC server is in warmup,
# but shut down before the call to getblockcount succeeds.
errno.ETIMEDOUT, # Treat identical to ECONNRESET
...
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909141475)
New version:
```python
# Suppress if cookie file isn't generated yet and no rpcuser or rpcpassword; bitcoind may be starting.
```
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909141475)
New version:
```python
# Suppress if cookie file isn't generated yet and no rpcuser or rpcpassword; bitcoind may be starting.
```
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908515569)
It seems wrong to me to attempt using the RPC connection in `stop_node()` when we known that `self.rpc_connected == False`. New version has:
```Python
if self.rpc_connected:
# Call stop-RPC...
elif force:
# Kill etc...
else:
raise RuntimeError(f"Cannot call stop-RPC as we don't have an RPC connection to process {self.process.pid}.")
```
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908515569)
It seems wrong to me to attempt using the RPC connection in `stop_node()` when we known that `self.rpc_connected == False`. New version has:
```Python
if self.rpc_connected:
# Call stop-RPC...
elif force:
# Kill etc...
else:
raise RuntimeError(f"Cannot call stop-RPC as we don't have an RPC connection to process {self.process.pid}.")
```
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909135121)
Went for option 3, making clear in the log message just above that a WARNING is expected.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909135121)
Went for option 3, making clear in the log message just above that a WARNING is expected.
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908463115)
Thanks for pointing out the race condition! It is not so likely if we get here because of RPC connection timeout, but still possible.
I tried adding a second `.kill()` after the first one and it does not raise an exception, and no exception is [documented](https://docs.python.org/3/library/subprocess.html#subprocess.Popen.kill).
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1908463115)
Thanks for pointing out the race condition! It is not so likely if we get here because of RPC connection timeout, but still possible.
I tried adding a second `.kill()` after the first one and it does not raise an exception, and no exception is [documented](https://docs.python.org/3/library/subprocess.html#subprocess.Popen.kill).
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909136687)
Added *test/functional/feature_framework_rpc_failure_details.py*.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909136687)
Added *test/functional/feature_framework_rpc_failure_details.py*.
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909139264)
Interesting, I do agree there is a spaghetti/complexity factor to `expected_ret_code`, so might revisit this.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909139264)
Interesting, I do agree there is a spaghetti/complexity factor to `expected_ret_code`, so might revisit this.
💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2580812651)
Updated based of ryanofsky's [review](https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324).
Hopefully latest PR summary is slightly clearer.
Replaced "test" -> "qa" since PR is about functional tests and not unit tests, should conform better with *CONTRIBUTING.md*.
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2580812651)
Updated based of ryanofsky's [review](https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324).
Hopefully latest PR summary is slightly clearer.
Replaced "test" -> "qa" since PR is about functional tests and not unit tests, should conform better with *CONTRIBUTING.md*.
💬 ryanofsky commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580824337)
> Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.
I think I don't understand what you are saying. If clients can connect to the node and call waitNext() to wait for block templates, then the node can "decide when it wants to push a new block, enabling predi
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580824337)
> Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.
I think I don't understand what you are saying. If clients can connect to the node and call waitNext() to wait for block templates, then the node can "decide when it wants to push a new block, enabling predi
...
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909184706)
I like the suggestion, but after implementing it still does not seem entirely ideal. I just made the entire `DBParams` field an optional, so it's not easily possible to set only the cache size, but not the path. It is also annoying that the current behaviour with the path is that it is dependent on the data dir path, not the blocks dir path.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909184706)
I like the suggestion, but after implementing it still does not seem entirely ideal. I just made the entire `DBParams` field an optional, so it's not easily possible to set only the cache size, but not the path. It is also annoying that the current behaviour with the path is that it is dependent on the data dir path, not the blocks dir path.
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909193453)
Good thinking ahead! Yeah, my idea was to use something along those lines too. Maybe we could start annotating some of the functions here to make it easier for the developer to see which exceptions it throws? Though my understanding is that such annotations have been deliberately left out of the code so far (https://stackoverflow.com/questions/1055387/throw-keyword-in-functions-signature).
On the other hand it would be nice if we could agree on more coherent error handling for constructors, b
...
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909193453)
Good thinking ahead! Yeah, my idea was to use something along those lines too. Maybe we could start annotating some of the functions here to make it easier for the developer to see which exceptions it throws? Though my understanding is that such annotations have been deliberately left out of the code so far (https://stackoverflow.com/questions/1055387/throw-keyword-in-functions-signature).
On the other hand it would be nice if we could agree on more coherent error handling for constructors, b
...
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2580871894)
Updated 384c73d4fcda4af7c2b4bfb945a248cb93dba47d -> b776e40554c8a315d832f3996d14ff2e3ae7b8cb ([blockmanDB_6](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_6) -> [blockmanDB_7](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_6..blockmanDB_7))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909124834), ran clang-format over commits.
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2580871894)
Updated 384c73d4fcda4af7c2b4bfb945a248cb93dba47d -> b776e40554c8a315d832f3996d14ff2e3ae7b8cb ([blockmanDB_6](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_6) -> [blockmanDB_7](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_6..blockmanDB_7))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1909124834), ran clang-format over commits.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909199419)
I did this to address https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301
Let me know if (`LARGE_TXS_COUNT` - 1) + `NORMAL_TXS_COUNT` is preferable.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909199419)
I did this to address https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301
Let me know if (`LARGE_TXS_COUNT` - 1) + `NORMAL_TXS_COUNT` is preferable.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201069)
I've taken your suggestion @Sjors.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201069)
I've taken your suggestion @Sjors.