💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2911607200)
Thanks for the feedback @purpleKarrot.
I like the sound of your approach here myself and will be happy to adapt to it. I think i'll start off by targetting 3.:
> A depends provider: This will ignore system packages and always use vendored depends. This approach will be used for reproducible builds.
...without the optionl "auto-build depends" functionality. This is close to what I have here already (and solves my immediate personal itch regarding building using depends on NixOS).
I wo
...
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2911607200)
Thanks for the feedback @purpleKarrot.
I like the sound of your approach here myself and will be happy to adapt to it. I think i'll start off by targetting 3.:
> A depends provider: This will ignore system packages and always use vendored depends. This approach will be used for reproducible builds.
...without the optionl "auto-build depends" functionality. This is close to what I have here already (and solves my immediate personal itch regarding building using depends on NixOS).
I wo
...
💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2108569954)
> Since this RPC allowed all other types to be aliases for watchonly stuff
I don't think it did. It should have only allowed bool:
```cpp
if (options.type() == UniValue::VBOOL) {
// backward compatibility bool only fallback
```
> this way it doesn't throw for any callers written in that way.
You removed it from the options object as well, which has strict=true checking enabled. So I think it would be more consistent to remove it here as well, or keep it for both
...
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2108569954)
> Since this RPC allowed all other types to be aliases for watchonly stuff
I don't think it did. It should have only allowed bool:
```cpp
if (options.type() == UniValue::VBOOL) {
// backward compatibility bool only fallback
```
> this way it doesn't throw for any callers written in that way.
You removed it from the options object as well, which has strict=true checking enabled. So I think it would be more consistent to remove it here as well, or keep it for both
...
🤔 polespinasa reviewed a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2870288028)
ACK
The test added faf6304bdfdf228354b4072b72f4c0ef90fdaade lgtm and makes much sense.
(non-blocking) My only point is that a warning message at least would be good for regtest as being at the tip and having the verification progress < 1 when no new blocks are getting mined (in regtest) may confuse.
<details>
<summary>see proposal
</summary>
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 880e35ee7f..b694171583 100644
--- a/src/rpc/blockchain.cpp
+++
...
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2870288028)
ACK
The test added faf6304bdfdf228354b4072b72f4c0ef90fdaade lgtm and makes much sense.
(non-blocking) My only point is that a warning message at least would be good for regtest as being at the tip and having the verification progress < 1 when no new blocks are getting mined (in regtest) may confuse.
<details>
<summary>see proposal
</summary>
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 880e35ee7f..b694171583 100644
--- a/src/rpc/blockchain.cpp
+++
...
👋 fanquake's pull request is ready for review: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439)
(https://github.com/bitcoin/bitcoin/pull/32439)
🤔 Kixunil reviewed a pull request: "contrib: add xor-blocks tool to obfuscate blocks directory"
(https://github.com/bitcoin/bitcoin/pull/32451#pullrequestreview-2870286845)
Sadly, this new implementation is quite broken in multiple ways.
(https://github.com/bitcoin/bitcoin/pull/32451#pullrequestreview-2870286845)
Sadly, this new implementation is quite broken in multiple ways.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108652853)
I see you're using this to display the messages from `main`. However that will lead to horrible messages containing `Error { msg: "..." }`. I suggest you implement `Debug` manually instead just displaying the string.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108652853)
I see you're using this to display the messages from `main`. However that will lead to horrible messages containing `Error { msg: "..." }`. I suggest you implement `Debug` manually instead just displaying the string.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108657220)
This loses error sources. The proper way to do this is to create a mutable string and then append the sources in a loop with `": "` as a separator. (You could also do cargo-style `"\n\tcaused by: "`).
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108657220)
This loses error sources. The proper way to do this is to create a mutable string and then append the sources in a loop with `": "` as a separator. (You could also do cargo-style `"\n\tcaused by: "`).
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108685983)
This kind of c-style handling is not idiomatic Rust. I'd do something like:
```rust
let mut args = std::env::args_os();
args.next().ok_or("Missing program name")?;
let data_dir_path = if let Some(first_arg) = args.next() {
let data_dir = if first_arg == "-datadir" || first_arg == "--datadir" {
args.next().ok_or("the datadir argument is missing a value")?
} else if let Some(data_dir) = first_arg.strip_prefix("-datadir=") {
data_dir.to_owned()
} else if let S
...
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108685983)
This kind of c-style handling is not idiomatic Rust. I'd do something like:
```rust
let mut args = std::env::args_os();
args.next().ok_or("Missing program name")?;
let data_dir_path = if let Some(first_arg) = args.next() {
let data_dir = if first_arg == "-datadir" || first_arg == "--datadir" {
args.next().ok_or("the datadir argument is missing a value")?
} else if let Some(data_dir) = first_arg.strip_prefix("-datadir=") {
data_dir.to_owned()
} else if let S
...
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108718168)
A comment explaining that this doesn't need to be cryptographic would be nice.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108718168)
A comment explaining that this doesn't need to be cryptographic would be nice.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108691581)
Reminds me that non-mainnet use paths like `/regtest/blocks`, perhaps we should accept `-chain` argument as well.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108691581)
Reminds me that non-mainnet use paths like `/regtest/blocks`, perhaps we should accept `-chain` argument as well.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108737971)
This seems over-complicated and not needed. You're not saing anything by "avoiding the conversion" - the optimizer would take care of that for you.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108737971)
This seems over-complicated and not needed. You're not saing anything by "avoiding the conversion" - the optimizer would take care of that for you.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108663194)
Using `args` is problematic for paths because it breaks for some values. `args_os` is the appropriate solution.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108663194)
Using `args` is problematic for paths because it breaks for some values. `args_os` is the appropriate solution.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108721393)
To be atomic, this should create a temporary file first, write it, sync it, and then rename.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108721393)
To be atomic, this should create a temporary file first, write it, sync it, and then rename.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108751295)
You need to call `flush()` before `into_inner`.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108751295)
You need to call `flush()` before `into_inner`.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2911912752)
> Unfortunately file locking is still experimental in Rust (?), so users would have to run nightly to be able to run the program. I think we should wait until it's stabilized instead of pulling in third party deps to do the locking.
You can just define `extern "C" {}` functions to call them the same way bitcoind does. It might even be the more correct approach anyway because there are three (!!!) kinds of locks on Unix and you must not mix them, and if Rust uses the same as bitcoind that's ju
...
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2911912752)
> Unfortunately file locking is still experimental in Rust (?), so users would have to run nightly to be able to run the program. I think we should wait until it's stabilized instead of pulling in third party deps to do the locking.
You can just define `extern "C" {}` functions to call them the same way bitcoind does. It might even be the more correct approach anyway because there are three (!!!) kinds of locks on Unix and you must not mix them, and if Rust uses the same as bitcoind that's ju
...
👍 hodlinator approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2870311100)
ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
### Concept
I understand node runners feeling like their agency would be restricted by deprecating and later removing this limit. However, I have not encountered any strong technical or incentive compatible argument in favor of keeping it. Deprecating and giving a little bit more opportunity for such an argument to appear before removal seems like a diplomatically acceptable solution.
I don't feel strongly enough about this to have kicked thi
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2870311100)
ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
### Concept
I understand node runners feeling like their agency would be restricted by deprecating and later removing this limit. However, I have not encountered any strong technical or incentive compatible argument in favor of keeping it. Deprecating and giving a little bit more opportunity for such an argument to appear before removal seems like a diplomatically acceptable solution.
I don't feel strongly enough about this to have kicked thi
...
💬 hodlinator commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2108669091)
nit: `MAX_STANDARD_TX_WEIGHT` (400000) is in vbytes AFAIK, but since you divide by `WITNESS_SCALE_FACTOR` the comment should say the unit is bytes (not vbytes), right?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2108669091)
nit: `MAX_STANDARD_TX_WEIGHT` (400000) is in vbytes AFAIK, but since you divide by `WITNESS_SCALE_FACTOR` the comment should say the unit is bytes (not vbytes), right?
💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2911944384)
Guix Build:
```bash
2d907a08701bbfa9a165581f69a997f5be0b3e0c7ee92f1bd5094df729430485 guix-build-ca9bb622af68/output/aarch64-linux-gnu/SHA256SUMS.part
f603a29e4eadeec26a7e82c882837d5369a35d65b3f283e50d72dd2fc7f9ba9d guix-build-ca9bb622af68/output/aarch64-linux-gnu/bitcoin-ca9bb622af68-aarch64-linux-gnu-debug.tar.gz
558bd5cc61134adb2c2a4120629288ffcd53162ffb1f787e95e56762340efce1 guix-build-ca9bb622af68/output/aarch64-linux-gnu/bitcoin-ca9bb622af68-aarch64-linux-gnu.tar.gz
23b3e836a56c8212
...
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2911944384)
Guix Build:
```bash
2d907a08701bbfa9a165581f69a997f5be0b3e0c7ee92f1bd5094df729430485 guix-build-ca9bb622af68/output/aarch64-linux-gnu/SHA256SUMS.part
f603a29e4eadeec26a7e82c882837d5369a35d65b3f283e50d72dd2fc7f9ba9d guix-build-ca9bb622af68/output/aarch64-linux-gnu/bitcoin-ca9bb622af68-aarch64-linux-gnu-debug.tar.gz
558bd5cc61134adb2c2a4120629288ffcd53162ffb1f787e95e56762340efce1 guix-build-ca9bb622af68/output/aarch64-linux-gnu/bitcoin-ca9bb622af68-aarch64-linux-gnu.tar.gz
23b3e836a56c8212
...
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2911998654)
If you reviewed the code by a commit hash, and you want to express your review, see [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process. (You'll have to include the commit hash in your review)
I'll leave the regtest stuff to a follow-up, so that it can be evaluated independently.
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2911998654)
If you reviewed the code by a commit hash, and you want to express your review, see [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process. (You'll have to include the commit hash in your review)
I'll leave the regtest stuff to a follow-up, so that it can be evaluated independently.
🤔 maflcko reviewed a pull request: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439#pullrequestreview-2870539601)
review ACK ca9bb622af68f0b89b1e5c8bc5726b60dec823a3
(https://github.com/bitcoin/bitcoin/pull/32439#pullrequestreview-2870539601)
review ACK ca9bb622af68f0b89b1e5c8bc5726b60dec823a3