💬 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
```
💬 achow101 commented on pull request "build, test: Build `db_tests.cpp` regardless of `USE_BDB`":
(https://github.com/bitcoin/bitcoin/pull/31617#issuecomment-2576143826)
ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
(https://github.com/bitcoin/bitcoin/pull/31617#issuecomment-2576143826)
ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991017)
Done
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991017)
Done
💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991117)
Removed
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991117)
Removed
💬 ryanofsky commented on issue "multiprocess: `ipc_tests` fail on NetBSD":
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2576154238)
I would guess based on the `std::system_error` "Invalid argument" error this failure should probably happen reliably instead of randomly, but hard to know.
It seems like from the logs `IpcPipeTest` is succeeding and the error is probably coming from one of the following tests `IpcSocketPairTest` or `IpcSocketTest` even though those tests do not appear to be printing anything. Maybe the reason they aren't printing anything is because they are using [`LogDebug`](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2576154238)
I would guess based on the `std::system_error` "Invalid argument" error this failure should probably happen reliably instead of randomly, but hard to know.
It seems like from the logs `IpcPipeTest` is succeeding and the error is probably coming from one of the following tests `IpcSocketPairTest` or `IpcSocketTest` even though those tests do not appear to be printing anything. Maybe the reason they aren't printing anything is because they are using [`LogDebug`](https://github.com/bitcoin/bitco
...
💬 davidgumberg commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2576159435)
trivial reACK https://github.com/bitcoin/bitcoin/commit/352391c2cf1a45231ae92ca92d2415b3786ab9ad
Recent push only changes comments.
<details>
<summary>
git diff 815467f..352391c
</summary>
```diff
$ git diff 815467f..352391c
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f884..4f5c38bb7b 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -186,7 +186,7 @@ using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
templa
...
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2576159435)
trivial reACK https://github.com/bitcoin/bitcoin/commit/352391c2cf1a45231ae92ca92d2415b3786ab9ad
Recent push only changes comments.
<details>
<summary>
git diff 815467f..352391c
</summary>
```diff
$ git diff 815467f..352391c
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f884..4f5c38bb7b 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -186,7 +186,7 @@ using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
templa
...
🤔 marcofleon reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2535162556)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Ran all the relevant tests, including the fuzz tests for about 50 cpu hours each. The implementation here of keeping track of multiple peers that announce an orphan looks good to me. It makes sense to not be locked in to only one peer for orphan resolution, as that peer could disconnect (in which case we lose the orphan) or be malicious.
Just left some non-blocking nits below.
Also quick question for my own understanding. Is it tr
...
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2535162556)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Ran all the relevant tests, including the fuzz tests for about 50 cpu hours each. The implementation here of keeping track of multiple peers that announce an orphan looks good to me. It makes sense to not be locked in to only one peer for orphan resolution, as that peer could disconnect (in which case we lose the orphan) or be malicious.
Just left some non-blocking nits below.
Also quick question for my own understanding. Is it tr
...