π l0rinc opened a pull request: "translations: recreate baseline to simplify testing"
(https://github.com/bitcoin/bitcoin/pull/33270)
### Summary
Previously, recalculating the translation string templates caused a misalignment: after any changed string, subsequent entries were re-numbered and old values received new βstableβ IDs.
You can reproduce by checking out https://github.com/bitcoin/bitcoin/pull/33224 and running:
```bash
cmake --preset dev-mode -DWITH_USDT=OFF -DENABLE_IPC=OFF && cmake --build build_dev_mode --target translate
```
This will change many translation IDs:
```bash
git diff | egrep '^[+-].+tra
...
(https://github.com/bitcoin/bitcoin/pull/33270)
### Summary
Previously, recalculating the translation string templates caused a misalignment: after any changed string, subsequent entries were re-numbered and old values received new βstableβ IDs.
You can reproduce by checking out https://github.com/bitcoin/bitcoin/pull/33224 and running:
```bash
cmake --preset dev-mode -DWITH_USDT=OFF -DENABLE_IPC=OFF && cmake --build build_dev_mode --target translate
```
This will change many translation IDs:
```bash
git diff | egrep '^[+-].+tra
...
π¬ l0rinc commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3236378613)
I have added a tool which could be useful for making sure edits like this aren't problematic anymore - feedback is welcome on overall direction: https://github.com/bitcoin/bitcoin/pull/33270
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3236378613)
I have added a tool which could be useful for making sure edits like this aren't problematic anymore - feedback is welcome on overall direction: https://github.com/bitcoin/bitcoin/pull/33270
π¬ hodlinator commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2309691268)
The line-counts where from the resulting `git diff`s. Admittedly they are imprecise due to surrounding context and other clang-format changes, but they give a rough relative estimate.
Thanks for vibing, almost double on same line. I'd say that does matter.
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2309691268)
The line-counts where from the resulting `git diff`s. Admittedly they are imprecise due to surrounding context and other clang-format changes, but they give a rough relative estimate.
Thanks for vibing, almost double on same line. I'd say that does matter.
π maflcko approved a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3167873647)
Looks like a nice change using more named `Arg` and `MaybeArg` helpers, removing the manual `params[idx].isNull` and `params[idx].get_str` legacy handling.
Not a blocker, but it would be nice to fix the theoretical c_str/data issue.
lgtm ACK 24d48638795ba61e8a420df4b831281455e208a7 π
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noA
...
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3167873647)
Looks like a nice change using more named `Arg` and `MaybeArg` helpers, removing the manual `params[idx].isNull` and `params[idx].get_str` legacy handling.
Not a blocker, but it would be nice to fix the theoretical c_str/data issue.
lgtm ACK 24d48638795ba61e8a420df4b831281455e208a7 π
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noA
...
π¬ maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309542222)
7b9a5d53c8ae47d2789e31306d035843d870f9a8: update docstring?
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309542222)
7b9a5d53c8ae47d2789e31306d035843d870f9a8: update docstring?
π¬ maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309677395)
```suggestion
std::string subnet_arg{help.Arg<std::string_view>("subnet"};
```
nit in the last commit: Use the named `Arg` helper for new code?
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309677395)
```suggestion
std::string subnet_arg{help.Arg<std::string_view>("subnet"};
```
nit in the last commit: Use the named `Arg` helper for new code?
π¬ maflcko commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3236477555)
Not sure if this needs a script. Post-freeze changes should be rare enough to just manually take over the affected string without going through any script.
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3236477555)
Not sure if this needs a script. Post-freeze changes should be rare enough to just manually take over the affected string without going through any script.
π¬ Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3236560911)
For `go-1.17.13.drv` I needed to manipulate the system clock again, as with https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2966089437
And as was the case there, `go-1.21.5` (`sudo date --set "05 dec 2023 00:00:00"`) fails with `AP_SYS_TIME unexpectedly not in the effective capability mask`. See https://github.com/golang/go/issues/67088#issuecomment-2081650479
But I forgot how to make guix download a substitute for this one single package...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3236560911)
For `go-1.17.13.drv` I needed to manipulate the system clock again, as with https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2966089437
And as was the case there, `go-1.21.5` (`sudo date --set "05 dec 2023 00:00:00"`) fails with `AP_SYS_TIME unexpectedly not in the effective capability mask`. See https://github.com/golang/go/issues/67088#issuecomment-2081650479
But I forgot how to make guix download a substitute for this one single package...
π¬ maflcko commented on pull request "doc: gen-manpages.py should check build options":
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309807425)
"Aborting generating manpages..." -> "Aborting generation of manpages..." [corrects ungrammatical gerund usage]
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309807425)
"Aborting generating manpages..." -> "Aborting generation of manpages..." [corrects ungrammatical gerund usage]
π¬ maflcko commented on pull request "doc: gen-manpages.py should check build options":
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309816260)
in theory, `--skip-missing-binaries` can now be removed and the list of binaries derived from the build options.
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309816260)
in theory, `--skip-missing-binaries` can now be removed and the list of binaries derived from the build options.
π¬ maflcko commented on pull request "doc: gen-manpages.py should check build options":
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309824605)
what is the point of listing those, which are unused?
(https://github.com/bitcoin/bitcoin/pull/33085#discussion_r2309824605)
what is the point of listing those, which are unused?
π¬ maflcko commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2309866024)
```suggestion
/** Guess background verification progress in case assume-utxo was used (as a fraction between 0.0=genesis and 1.0=snapshot blocks). */
double GetBackgroundVerificationProgress(const CBlockIndex& block) const EXCLUSIVE_LOCKS_REQUIRED(GetMutex());
```
can remove the unused nullptr handling here, as all call-sites already dereferenced the pointer
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2309866024)
```suggestion
/** Guess background verification progress in case assume-utxo was used (as a fraction between 0.0=genesis and 1.0=snapshot blocks). */
double GetBackgroundVerificationProgress(const CBlockIndex& block) const EXCLUSIVE_LOCKS_REQUIRED(GetMutex());
```
can remove the unused nullptr handling here, as all call-sites already dereferenced the pointer
π¬ maflcko commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2309861761)
"the heigh of the snapshot block" -> "the height of the snapshot block" [spelling error: "heigh" should be "height"]
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2309861761)
"the heigh of the snapshot block" -> "the height of the snapshot block" [spelling error: "heigh" should be "height"]
π¬ frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3236657195)
Updated `fuzz/difference_formatter.cpp` to directly use `BlockTransactionsRequest`
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3236657195)
Updated `fuzz/difference_formatter.cpp` to directly use `BlockTransactionsRequest`
π hodlinator approved a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3168171370)
ACK 74530ac9e05aafa91aaa00cc4f53bb4f26a55153
Clarifies the distinction between sigop-adjusted and non-sigop-adjusted vsize for RPC users.
Minor fixups and corrections since my last review https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3081014156
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3168171370)
ACK 74530ac9e05aafa91aaa00cc4f53bb4f26a55153
Clarifies the distinction between sigop-adjusted and non-sigop-adjusted vsize for RPC users.
Minor fixups and corrections since my last review https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3081014156
π¬ hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309759951)
info: Verified that Bitcoin Core seems to use sigop-adjusted size for block template construction as it sounds like that in this part. IIUC, we do, going by:
1. Checking the block is full with `packageSize`...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L209-L214
...which comes from `GetSizeWithAncestors()`...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L376-L390
...
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309759951)
info: Verified that Bitcoin Core seems to use sigop-adjusted size for block template construction as it sounds like that in this part. IIUC, we do, going by:
1. Checking the block is full with `packageSize`...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L209-L214
...which comes from `GetSizeWithAncestors()`...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L376-L390
...
π¬ hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309887787)
nit: I think this part is repetitive and somewhat arbitrary, currently the PR ensures sigop-adjusted and non-sigop-adjusted sizes are available for all mentioned RPCs. If I was using RPCs for each transaction for this calculation I would just pick the fastest one. Maybe I'm overlooking something though.
```suggestion
```
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309887787)
nit: I think this part is repetitive and somewhat arbitrary, currently the PR ensures sigop-adjusted and non-sigop-adjusted sizes are available for all mentioned RPCs. If I was using RPCs for each transaction for this calculation I would just pick the fastest one. Maybe I'm overlooking something though.
```suggestion
```
π¬ hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309870327)
nit: The adjusted beginning is nicer, but would rather bring back the end from 1523e8d6c981efff1394bd1669fbbbea5ff7ac97:
> Additionally, `getrawtransaction` now includes
+a `sigopsize` field and continues to report the unadjusted BIP-141 virtual size in its `vsize` field.
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2309870327)
nit: The adjusted beginning is nicer, but would rather bring back the end from 1523e8d6c981efff1394bd1669fbbbea5ff7ac97:
> Additionally, `getrawtransaction` now includes
+a `sigopsize` field and continues to report the unadjusted BIP-141 virtual size in its `vsize` field.
π¬ jsarenik commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236825770)
After it gets merged into `master` branch, I would recommend backporting it to current stable release branch which already contains the ephemeral outputs support ([29.x](https://github.com/bitcoin/bitcoin/blob/7cc9a087069bfcdb79a08ce77eb3a60adf9483af/doc/release-notes/release-notes-29.0.md?plain=1#L64}).
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236825770)
After it gets merged into `master` branch, I would recommend backporting it to current stable release branch which already contains the ephemeral outputs support ([29.x](https://github.com/bitcoin/bitcoin/blob/7cc9a087069bfcdb79a08ce77eb3a60adf9483af/doc/release-notes/release-notes-29.0.md?plain=1#L64}).
π¬ jsarenik commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3236886747)
Would such blocks mined by anyone even be considered valid on signet since they would not contain any expected valid signature?
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3236886747)
Would such blocks mined by anyone even be considered valid on signet since they would not contain any expected valid signature?