💬 jonatack commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575927475)
Valid addresses in RPC would be potentially more harmful due to the accidental risk of losing funds.
But in general, I believe the idea is to allow the RPC caller to invoke it with a value that is relevant to them / the use case of the software client, rather than an arbitrary bikesheddable real-life value, and by default to see an example of the RPC result with an invalid value provided.
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575927475)
Valid addresses in RPC would be potentially more harmful due to the accidental risk of losing funds.
But in general, I believe the idea is to allow the RPC caller to invoke it with a value that is relevant to them / the use case of the software client, rather than an arbitrary bikesheddable real-life value, and by default to see an example of the RPC result with an invalid value provided.
💬 l0rinc commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575939284)
I don't see how "id should be 64 chars long not 63" is helpful to the users - but I agree that placeholders could be more in-line with other more dangerous commands.
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575939284)
I don't see how "id should be 64 chars long not 63" is helpful to the users - but I agree that placeholders could be more in-line with other more dangerous commands.
💬 achow101 commented on pull request "[28.x] 28.1 backports and final changes":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2575949969)
ACK 36314b8da2ee65afd5636fa830d436c5c22bd260
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2575949969)
ACK 36314b8da2ee65afd5636fa830d436c5c22bd260
💬 Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1905885950)
That's true. I thought https://github.com/bitcoin/bitcoin/issues/31494 was focused on the best header chainwork not being up to minimumchainwork. I could change this to always skip script checks during a reindex, but IIUC having a block on disk doesn't mean that it was connected during the previous IBD. Might be worth it to sync headers first then connect blocks even during reindex.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1905885950)
That's true. I thought https://github.com/bitcoin/bitcoin/issues/31494 was focused on the best header chainwork not being up to minimumchainwork. I could change this to always skip script checks during a reindex, but IIUC having a block on disk doesn't mean that it was connected during the previous IBD. Might be worth it to sync headers first then connect blocks even during reindex.
🚀 achow101 merged a pull request: "[28.x] 28.1 backports and final changes"
(https://github.com/bitcoin/bitcoin/pull/31594)
(https://github.com/bitcoin/bitcoin/pull/31594)
🤔 ryanofsky reviewed a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324)
Code review 608b9ff097214bff3527cc240b7be313c62131d5
Nice changes in this PR. I've been confused by the error output from wait_for_rpc_connection() before, so think this will be helpful.
One thing that wasn't obvious to me from the PR description is that this improving error output in two separate ways:
- One change here is *adding* error output to `wait_for_rpc_connection`. Previously errors that happened during polling before the final "Unable to connect to bitcoind" error were silent
...
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324)
Code review 608b9ff097214bff3527cc240b7be313c62131d5
Nice changes in this PR. I've been confused by the error output from wait_for_rpc_connection() before, so think this will be helpful.
One thing that wasn't obvious to me from the PR description is that this improving error output in two separate ways:
- One change here is *adding* error output to `wait_for_rpc_connection`. Previously errors that happened during polling before the final "Unable to connect to bitcoind" error were silent
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904579994)
In commit "test refactor: wait_for_rpc_connection - Treat OSErrors the same" (5321eb089cd1b39160038050a6ef6f27fd6f4ef9)
Found both this comment "cookie file is missing" and previous comment "cookie file not found" confusing because it is not clear why the cookie file would not be present. IMO saying "node has not generated the cookie file yet" would be clearer.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904579994)
In commit "test refactor: wait_for_rpc_connection - Treat OSErrors the same" (5321eb089cd1b39160038050a6ef6f27fd6f4ef9)
Found both this comment "cookie file is missing" and previous comment "cookie file not found" confusing because it is not clear why the cookie file would not be present. IMO saying "node has not generated the cookie file yet" would be clearer.
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904607706)
In commit "test: Allow changing expected internal return code" (db0e1580a5212a162908ac4f23e935e1f76e06dc)
Now that this logic is getting more complicated with `self.expected_ret_code` I think the `expect_error` argument should be dropped so the code can be more understandable. It looks like there is only a single `expect_error` usage in `feature_abortnode.py` and it can be easily replaced by using `expected_ret_code`. Would suggest:
```diff
--- a/test/functional/feature_abortnode.py
+++
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904607706)
In commit "test: Allow changing expected internal return code" (db0e1580a5212a162908ac4f23e935e1f76e06dc)
Now that this logic is getting more complicated with `self.expected_ret_code` I think the `expect_error` argument should be dropped so the code can be more understandable. It looks like there is only a single `expect_error` usage in `feature_abortnode.py` and it can be easily replaced by using `expected_ret_code`. Would suggest:
```diff
--- a/test/functional/feature_abortnode.py
+++
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905811915)
In commit "test: Add feature_framework_rpc_failure.py" (f67d8963575feecdd4914c1a56bab4e0bebc000d)
Note: there is no test coverage for the "ignored errors" and "latest error" parts of the message. I think it would be good to add some coverage for this, even if it just checking for 0 and None, so there is a at least a hint that more coverage could be added here, especially since it's possible to imagine this functionality becoming broken in the future and more coverage helping prevent this.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905811915)
In commit "test: Add feature_framework_rpc_failure.py" (f67d8963575feecdd4914c1a56bab4e0bebc000d)
Note: there is no test coverage for the "ignored errors" and "latest error" parts of the message. I think it would be good to add some coverage for this, even if it just checking for 0 and None, so there is a at least a hint that more coverage could be added here, especially since it's possible to imagine this functionality becoming broken in the future and more coverage helping prevent this.
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905790290)
In commit "test: Add feature_framework_stop_node.py" (608b9ff097214bff3527cc240b7be313c62131d5)
One problem with this test as currently written is that it prints a real warning to the output (like `TestFramework.node0 (WARNING): Process 579831 already died with return code 1`). This is potentially confusing because this warning is expected and should not be a cause for concern, but you can't know that from looking at test output.
I can think of a few possible solutions:
- Redirecting lo
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905790290)
In commit "test: Add feature_framework_stop_node.py" (608b9ff097214bff3527cc240b7be313c62131d5)
One problem with this test as currently written is that it prints a real warning to the output (like `TestFramework.node0 (WARNING): Process 579831 already died with return code 1`). This is potentially confusing because this warning is expected and should not be a cause for concern, but you can't know that from looking at test output.
I can think of a few possible solutions:
- Redirecting lo
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904577914)
In commit "test refactor: wait_for_rpc_connection - Treat OSErrors the same" (5321eb089cd1b39160038050a6ef6f27fd6f4ef9)
I think these documentation changes look ok, but looking at old and new versions side by side I mostly found the previous comments easier to follow because they were more direct and interspersed with relevant code.
E.g. the previous "# Initialization phase" comment next to the exception being caught told me that the exception was expected during initialization. The curren
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904577914)
In commit "test refactor: wait_for_rpc_connection - Treat OSErrors the same" (5321eb089cd1b39160038050a6ef6f27fd6f4ef9)
I think these documentation changes look ok, but looking at old and new versions side by side I mostly found the previous comments easier to follow because they were more direct and interspersed with relevant code.
E.g. the previous "# Initialization phase" comment next to the exception being caught told me that the exception was expected during initialization. The curren
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905869855)
In commit "test: Allow changing expected internal return code" (db0e1580a5212a162908ac4f23e935e1f76e06dc)
Note: I like the changes in this commit, and think the new `TestNode.expected_ret_code` member could be a useful way to signal expected statuses when shutdown starts. But just want to point out that I don't think adding a new `expected_ret_code` member is strictly necessary for this PR to work. The only place it is actually used later in the PR is in the `stop_node` function when it is ki
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905869855)
In commit "test: Allow changing expected internal return code" (db0e1580a5212a162908ac4f23e935e1f76e06dc)
Note: I like the changes in this commit, and think the new `TestNode.expected_ret_code` member could be a useful way to signal expected statuses when shutdown starts. But just want to point out that I don't think adding a new `expected_ret_code` member is strictly necessary for this PR to work. The only place it is actually used later in the PR is in the `stop_node` function when it is ki
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904621833)
In commit "test: Kill process if stop-RPC is not available" (4ab7879ec6b71804cc3a51bcf58fa6318fa103b0)
This change doesn't seem safe. The stop_node method can be called by arbitrary test code at arbitrary times, including when `self.rpc_connected` is False, and I don't think it should be suppressing errors unless the caller has explicitly requested that errors should not be passed on.
Would suggest narrowing the error suppression behavior, so errors and bugs will be less likely to go unnot
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904621833)
In commit "test: Kill process if stop-RPC is not available" (4ab7879ec6b71804cc3a51bcf58fa6318fa103b0)
This change doesn't seem safe. The stop_node method can be called by arbitrary test code at arbitrary times, including when `self.rpc_connected` is False, and I don't think it should be suppressing errors unless the caller has explicitly requested that errors should not be passed on.
Would suggest narrowing the error suppression behavior, so errors and bugs will be less likely to go unnot
...
💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905685464)
In commit "test: Kill process if stop-RPC is not available" (4ab7879ec6b71804cc3a51bcf58fa6318fa103b0)
I think this is probably the simplest way to write this code and we should stick with it for now, but just want to point out there is a minor race condition here because the process could end between the time that poll and kill are called and then the kill call could throw an exception.
Another separate idea is that it might be safer to write `self.expected_ret_code = self.process.wait()`
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905685464)
In commit "test: Kill process if stop-RPC is not available" (4ab7879ec6b71804cc3a51bcf58fa6318fa103b0)
I think this is probably the simplest way to write this code and we should stick with it for now, but just want to point out there is a minor race condition here because the process could end between the time that poll and kill are called and then the kill call could throw an exception.
Another separate idea is that it might be safer to write `self.expected_ret_code = self.process.wait()`
...
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905948857)
Done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905948857)
Done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950341)
Nice catch, fixed but without hardcoding.
The comment above also needs to be updated, I did that.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950341)
Nice catch, fixed but without hardcoding.
The comment above also needs to be updated, I did that.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950486)
Done!
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950486)
Done!
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950615)
Done!
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905950615)
Done!
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905951876)
I think ex. is ambiguous so I shortened it by writing tx instead of transaction.
Also this is not logged frequently, only by miners I think.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1905951876)
I think ex. is ambiguous so I shortened it by writing tx instead of transaction.
Also this is not logged frequently, only by miners I think.
💬 ismaelsadeeq commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1905730379)
nit:
```
src/test/miner_tests.cpp:704: explictly ==> explicitly
```
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1905730379)
nit:
```
src/test/miner_tests.cpp:704: explictly ==> explicitly
```