💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2477872950)
@rkrux thanks for the heads up. I didn't expect changes to build system would affect this. I'll rebase asap, also want to bring in achow's [#22838](https://github.com/bitcoin/bitcoin/pull/22838)
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2477872950)
@rkrux thanks for the heads up. I didn't expect changes to build system would affect this. I'll rebase asap, also want to bring in achow's [#22838](https://github.com/bitcoin/bitcoin/pull/22838)
🤔 ryanofsky reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2437584042)
Code review 77c8a538b7164f8b65576600eb4668ac160f6b14. Thanks for the updates! This all looks great, my only substantial comments are about the RPC commit which I think is more complicated than it needs to be and may have a bug where it may allow an empty auth string to be accepted if -norpccookiefile is specified, or if there is an initialization failure.
It may be nice to add python functional tests to exercise some of the new behaviors. It would be nontrivial work to add test coverage for a
...
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2437584042)
Code review 77c8a538b7164f8b65576600eb4668ac160f6b14. Thanks for the updates! This all looks great, my only substantial comments are about the RPC commit which I think is more complicated than it needs to be and may have a bug where it may allow an empty auth string to be accepted if -norpccookiefile is specified, or if there is an initialization failure.
It may be nice to add python functional tests to exercise some of the new behaviors. It would be nontrivial work to add test coverage for a
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843140875)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)
I think having two separate accesses to the -rpccookiefile in different functions is fragile since `GetAuthCookieFile` will not do the right thing if called in a different context. It is also not taking advantage of GetPathArg's built in handling of negation. I think a change like:
```diff
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -86,6 +86,7 @@ static const char* const COOKIEAUTH_FILE = "
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843140875)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)
I think having two separate accesses to the -rpccookiefile in different functions is fragile since `GetAuthCookieFile` will not do the right thing if called in a different context. It is also not taking advantage of GetPathArg's built in handling of negation. I think a change like:
```diff
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -86,6 +86,7 @@ static const char* const COOKIEAUTH_FILE = "
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843188789)
In commit "args: Properly support -noconf and catch directories" (77c8a538b7164f8b65576600eb4668ac160f6b14)
It doesn't seem good to try to open a file with an empty path. This behavior doesn't seem well defined and the call will unnecessarily be passed down to the os when it shouldn't be. I think it would also clarify the code and avoid the need to add a second IsArgNegated call if this checked for an empty path. Would suggest:
```c++
const auto conf_path{GetConfigFilePath()};
st
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843188789)
In commit "args: Properly support -noconf and catch directories" (77c8a538b7164f8b65576600eb4668ac160f6b14)
It doesn't seem good to try to open a file with an empty path. This behavior doesn't seem well defined and the call will unnecessarily be passed down to the os when it shouldn't be. I think it would also clarify the code and avoid the need to add a second IsArgNegated call if this checked for an empty path. Would suggest:
```c++
const auto conf_path{GetConfigFilePath()};
st
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843179490)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)
The changes to `httprpc.cpp` seem too complicated and I don't think they are safe because now that an empty `strRPCUserColonPass` string is allowed by `RPCAuthorized`, the `TimingResistantEqual` function can return true when the client provides an empty auth string.
I also think changing `strRPCUserColonPass` to `std::optional` makes state more complicated and code more confusing to think about, even as i
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843179490)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)
The changes to `httprpc.cpp` seem too complicated and I don't think they are safe because now that an empty `strRPCUserColonPass` string is allowed by `RPCAuthorized`, the `TimingResistantEqual` function can return true when the client provides an empty auth string.
I also think changing `strRPCUserColonPass` to `std::optional` makes state more complicated and code more confusing to think about, even as i
...
👍 rkrux approved a pull request: "refactor: Avoid std::string format strings"
(https://github.com/bitcoin/bitcoin/pull/31287#pullrequestreview-2437724875)
tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
Make and unit tests successful. Checked the `printpriority` arg message in help.
```
-printpriority
Log transaction fee rate in BTC/kvB when mining blocks (default: 0)
```
(https://github.com/bitcoin/bitcoin/pull/31287#pullrequestreview-2437724875)
tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
Make and unit tests successful. Checked the `printpriority` arg message in help.
```
-printpriority
Log transaction fee rate in BTC/kvB when mining blocks (default: 0)
```
💬 maflcko commented on issue "valgrind: Conditional jump or move depends on uninitialised value(s)":
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2478073203)
The current CI with clang-16 seems to fail on arm64 anyway?
```
/ci_container_base/ci/scratch/build-aarch64-unknown-linux-gnu/src/test/test_bitcoin -t wallet_tests
```
```
==7187== Conditional jump or move depends on uninitialised value(s)
==7187== at 0xD363B4: wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) (./wallet/walletdb.cpp:1447)
==7187== by 0xCF6DCB: wallet::MakeWalletDatabase(std::__cxx11::basic_string<char,
...
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2478073203)
The current CI with clang-16 seems to fail on arm64 anyway?
```
/ci_container_base/ci/scratch/build-aarch64-unknown-linux-gnu/src/test/test_bitcoin -t wallet_tests
```
```
==7187== Conditional jump or move depends on uninitialised value(s)
==7187== at 0xD363B4: wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) (./wallet/walletdb.cpp:1447)
==7187== by 0xCF6DCB: wallet::MakeWalletDatabase(std::__cxx11::basic_string<char,
...
💬 maflcko commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2478167460)
If you want you can just open a new pull, it will have to be reviewed from scratch anyway.
> Are you still against passing through RANDOM_CTX_SEED in the test_runner.py? In that case I can drop that commit.
Yes, adding code like `fuzz_env['RANDOM_CTX_SEED'] = random_seed` to the fuzz runner also seems to go in the wrong direction a bit. The goal of the fuzz tests is to be fully deterministic and stable, within a single run and across different runs. If the fuzz randomness is seeded wit
...
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2478167460)
If you want you can just open a new pull, it will have to be reviewed from scratch anyway.
> Are you still against passing through RANDOM_CTX_SEED in the test_runner.py? In that case I can drop that commit.
Yes, adding code like `fuzz_env['RANDOM_CTX_SEED'] = random_seed` to the fuzz runner also seems to go in the wrong direction a bit. The goal of the fuzz tests is to be fully deterministic and stable, within a single run and across different runs. If the fuzz randomness is seeded wit
...
💬 maflcko commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1843352532)
Not sure about adding this. It seems more indirections to type `just configure -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ..etc...` than just `cmake -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ...etc...` directly.
In the end, I think for this to be useful, the developer or user would have to write the justfile themselves. Trying to offer one (even if it is just an example) is almost guaranteed to only find use by a single person.
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1843352532)
Not sure about adding this. It seems more indirections to type `just configure -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ..etc...` than just `cmake -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ...etc...` directly.
In the end, I think for this to be useful, the developer or user would have to write the justfile themselves. Trying to offer one (even if it is just an example) is almost guaranteed to only find use by a single person.
💬 Sjors commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1843367056)
I moved `destroy` to the top and renumbered.
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1843367056)
I moved `destroy` to the top and renumbered.
💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2478215295)
@ryanofsky & @luke-jr thanks for reviewing, will address feedback soon(tm).
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2478215295)
@ryanofsky & @luke-jr thanks for reviewing, will address feedback soon(tm).
💬 Sjors commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#issuecomment-2478222260)
@ismaelsadeeq are you referring to my statement at the end "so there's clients to break"?
Once we release the Mining interface, and multiprocess, people can build clients. If we then make changes in the next release, we could break those clients. But that's not an issue yet.
(https://github.com/bitcoin/bitcoin/pull/30391#issuecomment-2478222260)
@ismaelsadeeq are you referring to my statement at the end "so there's clients to break"?
Once we release the Mining interface, and multiprocess, people can build clients. If we then make changes in the next release, we could break those clients. But that's not an issue yet.
⚠️ vasild opened an issue: "Discover() will not run if listening on any address with an explicit bind=0.0.0.0"
(https://github.com/bitcoin/bitcoin/issues/31293)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
https://github.com/bitcoin/bitcoin/blob/85bcfeea23568053ea09013fb8263fa1511d7123/src/init.cpp#L1890-L1892
`Discover()` will run only if we are listening on all addresses (`bind_on_any` is `true`). However if `-bind=0.0.0.0:port` is explicitly given, then `bind_on_any` will end up being `false` and thus `Discover()` will not run when it should.
### Expected behaviour
Discover own addre
...
(https://github.com/bitcoin/bitcoin/issues/31293)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
https://github.com/bitcoin/bitcoin/blob/85bcfeea23568053ea09013fb8263fa1511d7123/src/init.cpp#L1890-L1892
`Discover()` will run only if we are listening on all addresses (`bind_on_any` is `true`). However if `-bind=0.0.0.0:port` is explicitly given, then `bind_on_any` will end up being `false` and thus `Discover()` will not run when it should.
### Expected behaviour
Discover own addre
...
💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2478267049)
Opened https://github.com/bitcoin/bitcoin/issues/31293 just that we don't forget about it.
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2478267049)
Opened https://github.com/bitcoin/bitcoin/issues/31293 just that we don't forget about it.
🤔 vasild reviewed a pull request: "net, init: derive default onion port if a user specified a -port"
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2438045624)
I reviewed the code and it looks safe and will achieve the intended purpose.
Since I introduced the current behavior which this PR is aiming to fix, I understand that I may have a bias towards downplaying the current issue (port collision) and a biased preference towards "It's fine, don't do anything". However, even with this realization, I can't help but think whether this will [introduce a bigger problem than the one it solves](https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2470
...
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2438045624)
I reviewed the code and it looks safe and will achieve the intended purpose.
Since I introduced the current behavior which this PR is aiming to fix, I understand that I may have a bias towards downplaying the current issue (port collision) and a biased preference towards "It's fine, don't do anything". However, even with this realization, I can't help but think whether this will [introduce a bigger problem than the one it solves](https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2470
...
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1843426357)
Maybe further elaborate this with an example:
If you are using `-port=` with non-standard value, for example `-port=5555` and not using `-bind=...=onion`, previously Bitcoin Core would listen for incoming Tor connections on `127.0.0.1:8334`. Now it would listen on `127.0.0.1:5556` (`-port` plus one). If you configured the hidden service manually in `torrc` now you have to change it from `HiddenServicePort 8333 127.0.0.1:8334` to `HiddenServicePort 8333 127.0.0.1:5556`, or configure `bitcoind`
...
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1843426357)
Maybe further elaborate this with an example:
If you are using `-port=` with non-standard value, for example `-port=5555` and not using `-bind=...=onion`, previously Bitcoin Core would listen for incoming Tor connections on `127.0.0.1:8334`. Now it would listen on `127.0.0.1:5556` (`-port` plus one). If you configured the hidden service manually in `torrc` now you have to change it from `HiddenServicePort 8333 127.0.0.1:8334` to `HiddenServicePort 8333 127.0.0.1:5556`, or configure `bitcoind`
...
👍 TheCharlatan approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2438142576)
ACK b28972cd85e4472b386349d6cda8c233faeffd4f
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2438142576)
ACK b28972cd85e4472b386349d6cda8c233faeffd4f
👍 rkrux approved a pull request: "test: group executed tests within the same directory"
(https://github.com/bitcoin/bitcoin/pull/31291#pullrequestreview-2438169141)
Concept ACK b7f40e8094e00c13a6c847340645b16704fed63b
(https://github.com/bitcoin/bitcoin/pull/31291#pullrequestreview-2438169141)
Concept ACK b7f40e8094e00c13a6c847340645b16704fed63b
💬 rkrux commented on pull request "test: group executed tests within the same directory":
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1843512387)
> Replicating the functional test framework behavior.
Should we use a prettier format here just like done in the functional tests? With this change the directory name is in plain nanoseconds like below.
```
drwx------ 3 rkrux staff 96B Nov 15 11:09 1731649167593349000
drwx------ 3 rkrux staff 96B Nov 15 11:11 1731649280506089000
```
Whereas for functional tests, the name is a little easier to read by using a date time format: https://github.com/bitcoin/bitcoin/blob/mast
...
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1843512387)
> Replicating the functional test framework behavior.
Should we use a prettier format here just like done in the functional tests? With this change the directory name is in plain nanoseconds like below.
```
drwx------ 3 rkrux staff 96B Nov 15 11:09 1731649167593349000
drwx------ 3 rkrux staff 96B Nov 15 11:11 1731649280506089000
```
Whereas for functional tests, the name is a little easier to read by using a date time format: https://github.com/bitcoin/bitcoin/blob/mast
...
💬 maflcko commented on pull request "test: group executed tests within the same directory":
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2478481544)
I think given that different processes (the normal case with `ctest -j$(nproc)`) still create different time dirs, I think it is also fine to leave this as-is. To me this is purely a style issue, so I don't mind either way.
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2478481544)
I think given that different processes (the normal case with `ctest -j$(nproc)`) still create different time dirs, I think it is also fine to leave this as-is. To me this is purely a style issue, so I don't mind either way.