📝 ajtowns opened a pull request: "rpc: Disallow captures in RPCMethodImpl"
(https://github.com/bitcoin/bitcoin/pull/34049)
When defining `RPCHelpMan` objects, we usually return a lambda, and mostly we define those via `[&](...) { ... }` which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don't), we would invoke undefined behaviour. So instead, convert all the `[&]` to `[]` to avoid capturing, and as part of `RPCHelpMan` check that the function we provide is convertible to a bare function pointer, so that any attempts to capture anything (even
...
(https://github.com/bitcoin/bitcoin/pull/34049)
When defining `RPCHelpMan` objects, we usually return a lambda, and mostly we define those via `[&](...) { ... }` which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don't), we would invoke undefined behaviour. So instead, convert all the `[&]` to `[]` to avoid capturing, and as part of `RPCHelpMan` check that the function we provide is convertible to a bare function pointer, so that any attempts to capture anything (even
...
💬 ajtowns commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2609006357)
Nice. Left the RPCResult text including both forms of output, as that makes it a little more self-documenting that people can use the deprecatedrpc option if desired. Also left `include` in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds running deprecatedrpc=logging.
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2609006357)
Nice. Left the RPCResult text including both forms of output, as that makes it a little more self-documenting that people can use the deprecatedrpc option if desired. Also left `include` in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds running deprecatedrpc=logging.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3640543665)
Very minor touch: moving of a curly nit-fix, rebase (noop); mostly to get CI to run again (had some apparently temporary issue).
(e2e25f4947e65869b014483be70b764a62bd0df7 -> 1a0423cf25bb60f98a29e06c39b7db2998860cd9)
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3640543665)
Very minor touch: moving of a curly nit-fix, rebase (noop); mostly to get CI to run again (had some apparently temporary issue).
(e2e25f4947e65869b014483be70b764a62bd0df7 -> 1a0423cf25bb60f98a29e06c39b7db2998860cd9)
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2609436783)
I kept the change minimal, but since the line is touched, I've adjusted the curly brace placement to be consistent, as suggested.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2609436783)
I kept the change minimal, but since the line is touched, I've adjusted the curly brace placement to be consistent, as suggested.
👍 maflcko approved a pull request: "rpc: Disallow captures in RPCMethodImpl"
(https://github.com/bitcoin/bitcoin/pull/34049#pullrequestreview-3566039894)
lgtm. Seems fine to remove this silent footgun. I left a nit in the second commit, because I think it is fine to relax it a bit.
Also, while doing a global scripted-diff here, might as well do the rename `RPCHelpMan` to `RPCMethod` from https://github.com/bitcoin/bitcoin/pull/19386#discussion_r447743298?
(https://github.com/bitcoin/bitcoin/pull/34049#pullrequestreview-3566039894)
lgtm. Seems fine to remove this silent footgun. I left a nit in the second commit, because I think it is fine to relax it a bit.
Also, while doing a global scripted-diff here, might as well do the rename `RPCHelpMan` to `RPCMethod` from https://github.com/bitcoin/bitcoin/pull/19386#discussion_r447743298?
💬 maflcko commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609441750)
nit in 48c3c41fa604b8634c509bb38d67de9bea2ab59d: I think the first commit makes a lot of sense and should be done. However, the second commit seems overly restrictive. Here, it is easy to re-create the calculation, but will it always be the case? I think it could make sense to either:
* drop the second commit, and let asan/valgrind disallow the dangling references, like they have done in the past
* Clarify the comment around the static_assert which is hit at compile time to say that any capture
...
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609441750)
nit in 48c3c41fa604b8634c509bb38d67de9bea2ab59d: I think the first commit makes a lot of sense and should be done. However, the second commit seems overly restrictive. Here, it is easy to re-create the calculation, but will it always be the case? I think it could make sense to either:
* drop the second commit, and let asan/valgrind disallow the dangling references, like they have done in the past
* Clarify the comment around the static_assert which is hit at compile time to say that any capture
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482205)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482205)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482375)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482375)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482472)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609482472)
https://github.com/bitcoin/bitcoin/commit/9f8809f2b1206ad34dfc68831c10db76e618de3f
💬 maflcko commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2609578248)
the file isn't left around. This was fixed already in fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12
(https://github.com/bitcoin/bitcoin/pull/34046#discussion_r2609578248)
the file isn't left around. This was fixed already in fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12
💬 maflcko commented on pull request "cmake: Add fail pattern to `try_append_cxx_flags`":
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2609591510)
i don't understand how this could lead to issues. If someone passes w-no-error during configure, it will also be present during build, and the build should pass, no?
What is the issues that this is trying to fix?
What are the steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/34047#discussion_r2609591510)
i don't understand how this could lead to issues. If someone passes w-no-error during configure, it will also be present during build, and the build should pass, no?
What is the issues that this is trying to fix?
What are the steps to reproduce?
💬 maflcko commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2609597196)
i presume this exists to allow indexes to obfuscate, if there is need to? E.g. when storing remote-user-provided data. E.g. an addrindex?
I understand this isn't needed right now, so no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/34048#discussion_r2609597196)
i presume this exists to allow indexes to obfuscate, if there is need to? E.g. when storing remote-user-provided data. E.g. an addrindex?
I understand this isn't needed right now, so no strong opinion.
💬 maflcko commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2609640100)
running -> run [The sentence starts with an imperative ("Uninstall Guix itself...") and needs a parallel imperative verb; "or running" is a fragment — "or run" makes the coordination grammatical.]
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2609640100)
running -> run [The sentence starts with an imperative ("Uninstall Guix itself...") and needs a parallel imperative verb; "or running" is a fragment — "or run" makes the coordination grammatical.]
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609654918)
Not sure about the `try`/`finally`. I think it may be better to leave the filesystem as-is in case of failure, so that devs inspecting the directory of a failed test finds it closer to how it looked when the failure happened. Haven't seen this kind of pattern in other functional tests. We don't continue the test regardless, so there's no benefit to checks later in the test.
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -500,11 +500,9 @@ class REST
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609654918)
Not sure about the `try`/`finally`. I think it may be better to leave the filesystem as-is in case of failure, so that devs inspecting the directory of a failed test finds it closer to how it looked when the failure happened. Haven't seen this kind of pattern in other functional tests. We don't continue the test regardless, so there's no benefit to checks later in the test.
```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -500,11 +500,9 @@ class REST
...
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609627988)
nit: In my opinion it would be better to use `REQUIRE` which stops the test in case of failure, so that we don't trigger less clear failures on the following lines (internal assert in `Expected::value()` - might be an exception after #34032, assert inside `Expected::error()`).
```diff
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -152,7 +152,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_part, TestChain100Setup)
const auto expect_part{[&](siz
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2609627988)
nit: In my opinion it would be better to use `REQUIRE` which stops the test in case of failure, so that we don't trigger less clear failures on the following lines (internal assert in `Expected::value()` - might be an exception after #34032, assert inside `Expected::error()`).
```diff
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -152,7 +152,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_part, TestChain100Setup)
const auto expect_part{[&](siz
...
💬 Fi3 commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3640907952)
> reply to [@Fi3](https://github.com/Fi3):
>
> first, let me explain the rationale behind this feature request
>
> once `DeclareMiningJob` arrives, I wanted JDS to be able to validate it as fast as possible
>
> assuming the best case scenario (where all `wtxid`s have already been seen and no extra `ProvideMissingTransactions` round-trip is needed), having all txs readily available on JDS memory would allow the validation of `DeclareMiningJob` to happen ASAP
>
> if we're worried about unbound
...
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3640907952)
> reply to [@Fi3](https://github.com/Fi3):
>
> first, let me explain the rationale behind this feature request
>
> once `DeclareMiningJob` arrives, I wanted JDS to be able to validate it as fast as possible
>
> assuming the best case scenario (where all `wtxid`s have already been seen and no extra `ProvideMissingTransactions` round-trip is needed), having all txs readily available on JDS memory would allow the validation of `DeclareMiningJob` to happen ASAP
>
> if we're worried about unbound
...
💬 ajtowns commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609754446)
> Here, it is easy to re-create the calculation, but will it always be the case?
I think so -- the `RpcMethodFnType` functions don't take any params, so if you're doing a calculation (like a deprecatedrpc test) you can just do the same thing inside the `RPCMethodImpl`, and if you're providing something hardcoded, you can pass a boolean or enum via a template (eg `template <bool wants_enum> static RPCHelpMan bumpfee_filter`) and make your logic conditional on that.
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609754446)
> Here, it is easy to re-create the calculation, but will it always be the case?
I think so -- the `RpcMethodFnType` functions don't take any params, so if you're doing a calculation (like a deprecatedrpc test) you can just do the same thing inside the `RPCMethodImpl`, and if you're providing something hardcoded, you can pass a boolean or enum via a template (eg `template <bool wants_enum> static RPCHelpMan bumpfee_filter`) and make your logic conditional on that.
💬 Ataraxia009 commented on pull request "rpc: Disallow captures in RPCMethodImpl":
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609757315)
I dont think the second commit is necessary either should be fine with just removal of the captures
(https://github.com/bitcoin/bitcoin/pull/34049#discussion_r2609757315)
I dont think the second commit is necessary either should be fine with just removal of the captures
💬 Ataraxia009 commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3640958735)
Concept NAck, could be needed in the future, no point in hiding it
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3640958735)
Concept NAck, could be needed in the future, no point in hiding it
🤔 hodlinator reviewed a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566334104)
re-ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566334104)
re-ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb