💬 fanquake commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720590640)
Concept NACK on introducing build options to control building single binaries.
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720590640)
Concept NACK on introducing build options to control building single binaries.
💬 hebasto commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720603006)
> Concept NACK on introducing build options to control building single binaries.
Every other `BUILD_*` variable does exactly the same.
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720603006)
> Concept NACK on introducing build options to control building single binaries.
Every other `BUILD_*` variable does exactly the same.
💬 hebasto commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720618613)
Alternatively, an explicit list of targets could be specified here:https://github.com/bitcoin/bitcoin/blob/c20a5ce106be716a503bcf615a1900ba8c635430/contrib/guix/libexec/build.sh#L247-L248
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720618613)
Alternatively, an explicit list of targets could be specified here:https://github.com/bitcoin/bitcoin/blob/c20a5ce106be716a503bcf615a1900ba8c635430/contrib/guix/libexec/build.sh#L247-L248
💬 fanquake commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720619271)
Sure, but I think that isn't a great pattern, and this makes it worse. Having a `BUILD_TESTS` & `BUILD_TEST_BINARY` is awkward, and I think the motivation is poor; if you only want to build certain binaries in a Guix build, then use `--target`. Why does it need a new build option?
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720619271)
Sure, but I think that isn't a great pattern, and this makes it worse. Having a `BUILD_TESTS` & `BUILD_TEST_BINARY` is awkward, and I think the motivation is poor; if you only want to build certain binaries in a Guix build, then use `--target`. Why does it need a new build option?
👍 laanwj approved a pull request: "depends: remove `NO_HARDEN` option"
(https://github.com/bitcoin/bitcoin/pull/32038#pullrequestreview-2681205220)
Code review ACK 5dfef6b9b379f51e69f2a358c05ae3c3e8a26e13
(https://github.com/bitcoin/bitcoin/pull/32038#pullrequestreview-2681205220)
Code review ACK 5dfef6b9b379f51e69f2a358c05ae3c3e8a26e13
💬 laanwj commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2720673981)
Concept ACK, this has been a source of frustration and misunderstanding in bitcoin's code since the beginning. Hashes are byte blobs, not numbers, and unless there's a specific good reason to interpret them as a number (can't think of more cases than those you already mention) they shouldn't be.
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2720673981)
Concept ACK, this has been a source of frustration and misunderstanding in bitcoin's code since the beginning. Hashes are byte blobs, not numbers, and unless there's a specific good reason to interpret them as a number (can't think of more cases than those you already mention) they shouldn't be.
💬 Sjors commented on issue "Add test coverage for getblocktemplate fees":
(https://github.com/bitcoin/bitcoin/issues/32053#issuecomment-2720689733)
I ended up writing one for #31897. Can be salvaged if that PR doesn't make it.
(https://github.com/bitcoin/bitcoin/issues/32053#issuecomment-2720689733)
I ended up writing one for #31897. Can be salvaged if that PR doesn't make it.
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993185874)
Pushed a test.
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993185874)
Pushed a test.
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2720701557)
Rebased and switched the order of commits, so it's more clear this PR doesn't change behavior.
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2720701557)
Rebased and switched the order of commits, so it's more clear this PR doesn't change behavior.
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1993202306)
I've updated code block. Could you please check @maflcko and @davidrobinsonau
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1993202306)
I've updated code block. Could you please check @maflcko and @davidrobinsonau
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720735149)
The current approach of having the Template Provider communicate via IPC, as described in #31098, indeed does not require Bitcoin Core to change its net code.
Making easy to copy-paste code could still be a motivation for the author, but it would not a good reason to include it.
Additionally, as @theuni pointed out in https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374, it may be the right design / abstraction for a Template Provider either.
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720735149)
The current approach of having the Template Provider communicate via IPC, as described in #31098, indeed does not require Bitcoin Core to change its net code.
Making easy to copy-paste code could still be a motivation for the author, but it would not a good reason to include it.
Additionally, as @theuni pointed out in https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374, it may be the right design / abstraction for a Template Provider either.
💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720738307)
> @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
>
> * Is your result different with or without the last commit?
> * Does `ResetCoverageCounters` work?
> * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)
In checking this issue I discovered that due to the
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720738307)
> @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
>
> * Is your result different with or without the last commit?
> * Does `ResetCoverageCounters` work?
> * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)
In checking this issue I discovered that due to the
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720745456)
> I discovered that due to the change in [31161](https://github.com/bitcoin/bitcoin/pull/31161) the tooling is broken. (the path has changed from `src/test/` to `/bin` )
Should be trivial to fix up. I am happy to review a tiny pull to fix the silent merge conflict here.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720745456)
> I discovered that due to the change in [31161](https://github.com/bitcoin/bitcoin/pull/31161) the tooling is broken. (the path has changed from `src/test/` to `/bin` )
Should be trivial to fix up. I am happy to review a tiny pull to fix the silent merge conflict here.
👍 rkrux approved a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2681359199)
tACK a24419f8bed5e1145ce171dbbdad957750585471
I can see it adds the previously excluded options now (starting with `alertnotify`) in the example conf file.
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2681359199)
tACK a24419f8bed5e1145ce171dbbdad957750585471
I can see it adds the previously excluded options now (starting with `alertnotify`) in the example conf file.
💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2720804983)
> It seems that the first thing to decide is the source of the version information:
>
> * the `CLIENT_VERSION_*` variables set by the build system, or
>
> * a GitHub tag
A related discussion happens in https://github.com/bitcoin-core/bitcoin-maintainer-tools/issues/178.
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2720804983)
> It seems that the first thing to decide is the source of the version information:
>
> * the `CLIENT_VERSION_*` variables set by the build system, or
>
> * a GitHub tag
A related discussion happens in https://github.com/bitcoin-core/bitcoin-maintainer-tools/issues/178.
💬 saikiran57 commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993261238)
My primary goal is to give fix with minor changes to code, that's why I've updated this `throw std::runtime_error(std::string(__func__) + ": " + error);` to `throw std::invalid_argument(error);` so it does its satisfy the test case. During rpc call if we face issue I cached this exception(invalid_argument) return with existing response message to user if any other exception happened I just left with default implementation.
Regarding your suggestion using `util::Result<void>`. Inside AddWallet
...
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993261238)
My primary goal is to give fix with minor changes to code, that's why I've updated this `throw std::runtime_error(std::string(__func__) + ": " + error);` to `throw std::invalid_argument(error);` so it does its satisfy the test case. During rpc call if we face issue I cached this exception(invalid_argument) return with existing response message to user if any other exception happened I just left with default implementation.
Regarding your suggestion using `util::Result<void>`. Inside AddWallet
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2720833753)
Rebased to resolve a silent merge conflict with merged bitcoin/bitcoin#31161.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2720833753)
Rebased to resolve a silent merge conflict with merged bitcoin/bitcoin#31161.
🤔 l0rinc requested changes to a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2681370790)
I like these cleanups, ans we should go a step further and modernize the code more when touching them.
I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2681370790)
I like these cleanups, ans we should go a step further and modernize the code more when touching them.
I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993237690)
This is better than before, but having a `NETINFO_MAX_LEVEL` and a checking the bounded values in methods and having `DetailsRequested`, but also checking it manually in https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L499-L501 where we're checking the raw `n` value seems quite hacky.
Seems the code is begging for a bounded type - if we're cleaning it up, we might as well try an enum here:
```C++
enum class NetinfoDetailLevel : uint8_t {
BASIC = 0,
PEERS,
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993237690)
This is better than before, but having a `NETINFO_MAX_LEVEL` and a checking the bounded values in methods and having `DetailsRequested`, but also checking it manually in https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L499-L501 where we're checking the raw `n` value seems quite hacky.
Seems the code is begging for a bounded type - if we're cleaning it up, we might as well try an enum here:
```C++
enum class NetinfoDetailLevel : uint8_t {
BASIC = 0,
PEERS,
...
💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993269714)
simple structs are fine, but I'm not a fan of struct inheritance, I like the previous version more in the non-trivial cases
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993269714)
simple structs are fine, but I'm not a fan of struct inheritance, I like the previous version more in the non-trivial cases
💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993256631)
from the usages it seems we can modernize this in a few more ways:
* `UNKNOWN_NETWORK` seems to serve as a value-missing proxy - we could use an optional instead
* `const std::string&` could be modernized to `static std::optional<int8_t> NetworkStringToId(std::string_view network)`
This would allow us to change the usages to something like:
```C++
if (auto network_id{NetworkStringToId(network_name)}) {
++counts.at(*network_id);
}
```
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993256631)
from the usages it seems we can modernize this in a few more ways:
* `UNKNOWN_NETWORK` seems to serve as a value-missing proxy - we could use an optional instead
* `const std::string&` could be modernized to `static std::optional<int8_t> NetworkStringToId(std::string_view network)`
This would allow us to change the usages to something like:
```C++
if (auto network_id{NetworkStringToId(network_name)}) {
++counts.at(*network_id);
}
```