💬 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.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201883)
I've taken this with a little modification.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909201883)
I've taken this with a little modification.
👋 brunoerg's pull request is ready for review: "descriptor: check whitespace in pubkeys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603)
(https://github.com/bitcoin/bitcoin/pull/31603)
💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2580884607)
Ready for review!
Thanks, @furszy for the comment. I've changed the approched and described it on description.
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2580884607)
Ready for review!
Thanks, @furszy for the comment. I've changed the approched and described it on description.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208594)
> This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.
> We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.
I've reverted and included it but clarify the `getmininginfo` help text.
> I agree we should not change the `weight`, `currentblockweight` and `sigops` fields in `getblocktemplate` and `getmi
...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208594)
> This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.
> We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.
I've reverted and included it but clarify the `getmininginfo` help text.
> I agree we should not change the `weight`, `currentblockweight` and `sigops` fields in `getblocktemplate` and `getmi
...
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208803)
Done.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208803)
Done.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2580905232)
Thanks for reviewing @pinheadmz @luke-jr @Sjors
I've recently foreced pushed from 2fb833e69627a8f9b1c4b14b60d1df9903adf210 to 7ab1344eb13228998bcead7328d5cc0a905971d4 [diff](https://github.com/bitcoin/bitcoin/compare/2fb833e69627a8f9b1c4b14b60d1df9903adf210..7ab1344eb13228998bcead7328d5cc0a905971d4)
Changes were:
1. Reverting https://github.com/bitcoin/bitcoin/commit/76a95522b322c1a8ab1594ca8bf5ac99ab379c46 but improving the help text.
2. Use policy `DEFAULT_MAX_BLOCK_WEIGHT` instead
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2580905232)
Thanks for reviewing @pinheadmz @luke-jr @Sjors
I've recently foreced pushed from 2fb833e69627a8f9b1c4b14b60d1df9903adf210 to 7ab1344eb13228998bcead7328d5cc0a905971d4 [diff](https://github.com/bitcoin/bitcoin/compare/2fb833e69627a8f9b1c4b14b60d1df9903adf210..7ab1344eb13228998bcead7328d5cc0a905971d4)
Changes were:
1. Reverting https://github.com/bitcoin/bitcoin/commit/76a95522b322c1a8ab1594ca8bf5ac99ab379c46 but improving the help text.
2. Use policy `DEFAULT_MAX_BLOCK_WEIGHT` instead
...
💬 theuni commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580925856)
> > 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?
Not change for change's sake, no. Please consider this from the POV of reviewers and maintainers. A constant stream of changes like that is exhausting. It would distract from larger improvements.
I think @maflcko's comment above is a good d
...
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580925856)
> > 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?
Not change for change's sake, no. Please consider this from the POV of reviewers and maintainers. A constant stream of changes like that is exhausting. It would distract from larger improvements.
I think @maflcko's comment above is a good d
...
📝 mzumsande opened a pull request: "wallet: fix rescanning inconsistency"
(https://github.com/bitcoin/bitcoin/pull/31629)
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.
This means that
...
(https://github.com/bitcoin/bitcoin/pull/31629)
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.
This means that
...