💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070718791)
Right, it checks whether the expected result and the selected input set match, but the message here is printed in the case of a failure!
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070718791)
Right, it checks whether the expected result and the selected input set match, but the message here is printed in the case of a failure!
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620399)
> I was referring to something like the code below (which encapsulates the latest commit changes), but it was just a suggestion. The current code looks good to me too.
I see, thanks. I guess it could be nice to be able to run the test suite in smaller portions especially if some of the tests took a long time, but it overall runs extremely quickly, so I’m not sure it is necessary to further structure the tests at this time.
I intend to work on porting the other algorithms’ tests in the same
...
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620399)
> I was referring to something like the code below (which encapsulates the latest commit changes), but it was just a suggestion. The current code looks good to me too.
I see, thanks. I guess it could be nice to be able to run the test suite in smaller portions especially if some of the tests took a long time, but it overall runs extremely quickly, so I’m not sure it is necessary to further structure the tests at this time.
I intend to work on porting the other algorithms’ tests in the same
...
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620960)
Should be ready to review, again.
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620960)
Should be ready to review, again.
💬 instagibbs commented on issue "p2p: lingering entries in `mapBlockSource`":
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-2845639973)
If we don't think a whole fix is in the cards soon, I'm partial to adding a test mechanically checking the weird behavior.
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-2845639973)
If we don't think a whole fix is in the cards soon, I'm partial to adding a test mechanically checking the weird behavior.
👍 pinheadmz approved a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2810346702)
ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
Built and ran tests on arm64/macos. Played with the feature locally. Reviewed each commit, some questions below.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgT0ykACgkQ5+KYS2KJ
yTogNQ/+NxdemF18NTDuoW6W7fLeDbdbJCbe/9fSse0fhFi0rtTkFpJdXKt+p86T
gndlLfTNitJ58BUul5WMbJ
...
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2810346702)
ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
Built and ran tests on arm64/macos. Played with the feature locally. Reviewed each commit, some questions below.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgT0ykACgkQ5+KYS2KJ
yTogNQ/+NxdemF18NTDuoW6W7fLeDbdbJCbe/9fSse0fhFi0rtTkFpJdXKt+p86T
gndlLfTNitJ58BUul5WMbJ
...
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845)
ea98a42640b9ff77a462e55887025ddd1da54727
Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845)
ea98a42640b9ff77a462e55887025ddd1da54727
Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693)
ea98a42640b9ff77a462e55887025ddd1da54727
I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
I guess you need to parse the request to get the method before checking the whitelist, but I just wonder i
...
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693)
ea98a42640b9ff77a462e55887025ddd1da54727
I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
I guess you need to parse the request to get the method before checking the whitelist, but I just wonder i
...
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369)
ea98a42640b9ff77a462e55887025ddd1da54727
Might want to indicate `status` is an "out" param?
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369)
ea98a42640b9ff77a462e55887025ddd1da54727
Might want to indicate `status` is an "out" param?
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092)
113b46d31075c66cecbe4482aed362d50cdec28c
I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
What about the try/catch? There's no "connection failed" case like there is for HTTP?
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092)
113b46d31075c66cecbe4482aed362d50cdec28c
I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
What about the try/catch? There's no "connection failed" case like there is for HTTP?
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905)
9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
Hm for some reason I thought it could be as low as 92:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/test/functional/feature_proxy.py#L66-L67
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905)
9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
Hm for some reason I thought it could be as low as 92:
https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/test/functional/feature_proxy.py#L66-L67
💬 davidgumberg commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2845668503)
I think #31250 genuinely fixed this, https://github.com/bitcoin/bitcoin/pull/31250/commits/c847dee1488a294c9a9632a00ba1134b21e41947, changed the logic for whether or not tests are run in all the crash cases exhibited here:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index c7f0cc5e43..ee79a27392 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -567,7 +565,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert
...
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2845668503)
I think #31250 genuinely fixed this, https://github.com/bitcoin/bitcoin/pull/31250/commits/c847dee1488a294c9a9632a00ba1134b21e41947, changed the logic for whether or not tests are run in all the crash cases exhibited here:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index c7f0cc5e43..ee79a27392 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -567,7 +565,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert
...
💬 monlovesmango commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070755965)
Ok I see! I understand now. Sorry was running with `--log_level=all` and was misinterpreting these as success messages. Please disregard..
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070755965)
Ok I see! I understand now. Sorry was running with `--log_level=all` and was misinterpreting these as success messages. Please disregard..
🤔 1440000bytes reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2810654144)
utACK https://github.com/bitcoin/bitcoin/pull/29365/commits/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1
Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2810654144)
utACK https://github.com/bitcoin/bitcoin/pull/29365/commits/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1
Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
💬 hebasto commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2845818802)
> > During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
>
> Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.
https://github.com/arun11299/cpp-sub
...
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2845818802)
> > During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
>
> Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.
https://github.com/arun11299/cpp-sub
...
💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2845902580)
> utACK [41728b5](https://github.com/bitcoin/bitcoin/commit/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1)
>
> Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
It is possible to split the commit (and the commit message) into multiple parts:
- add function `ParseWrappedSignetChallenge` and its unit test (files `src/chainparams.{h,cpp} src/test/chainparams_tests.cpp src/test/CMakeLists.txt`)
- add function `ParseSignetParams` and its unit t
...
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2845902580)
> utACK [41728b5](https://github.com/bitcoin/bitcoin/commit/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1)
>
> Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
It is possible to split the commit (and the commit message) into multiple parts:
- add function `ParseWrappedSignetChallenge` and its unit test (files `src/chainparams.{h,cpp} src/test/chainparams_tests.cpp src/test/CMakeLists.txt`)
- add function `ParseSignetParams` and its unit t
...
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070888721)
Thanks for taking such a thorough look!
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070888721)
Thanks for taking such a thorough look!
💬 davidgumberg commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2070888806)
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)
Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers `shared_ptr`, e.g. https://github.com/davidgumberg/bitcoin/commit/75c839b8d77c207a0946294672b703d9f8eecbe1
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2070888806)
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)
Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers `shared_ptr`, e.g. https://github.com/davidgumberg/bitcoin/commit/75c839b8d77c207a0946294672b703d9f8eecbe1
💬 oxqnd commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845930776)
I’ll take a look at #31298, but it might take me a little while to get to it. Hope that’s okay!
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845930776)
I’ll take a look at #31298, but it might take me a little while to get to it. Hope that’s okay!
📝 mzumsande opened a pull request: "cli: add -usefile option"
(https://github.com/bitcoin/bitcoin/pull/32399)
Running large commands via `bitcoin-cli` (such as submitting a large tx or block) runs into the `MAX_ARG_STRLEN` limit, which is 128KB on many systems. (see e.g. https://trofi.github.io/posts/299-maximum-argument-count-on-linux-and-in-gcc.html for more info).
This PR suggests to make it possible to bypass this limit by allowing to put the command and all of its args into a file. The functional test framework is then adjusted to automatically use this option (using a temporary file) if `--usec
...
(https://github.com/bitcoin/bitcoin/pull/32399)
Running large commands via `bitcoin-cli` (such as submitting a large tx or block) runs into the `MAX_ARG_STRLEN` limit, which is 128KB on many systems. (see e.g. https://trofi.github.io/posts/299-maximum-argument-count-on-linux-and-in-gcc.html for more info).
This PR suggests to make it possible to bypass this limit by allowing to put the command and all of its args into a file. The functional test framework is then adjusted to automatically use this option (using a temporary file) if `--usec
...
💬 oxqnd commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845932631)
> > Could I try this issue?
>
> Would you mind reviewing https://github.com/bitcoin/bitcoin/pull/31298 instead?
>
> This solution is still up to date, it just hasn't gotten the right reviewers.
I’ll take a look at #31298, but it might take me a little while to get to it. Hope that’s okay!
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845932631)
> > Could I try this issue?
>
> Would you mind reviewing https://github.com/bitcoin/bitcoin/pull/31298 instead?
>
> This solution is still up to date, it just hasn't gotten the right reviewers.
I’ll take a look at #31298, but it might take me a little while to get to it. Hope that’s okay!