💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095077056)
Should we really continue after reading an invalid key length? Feels like it would be good to throw an exception, or `assert` the condition instead of `if`.
(Could adjust the message/comment to include the possibility of us being old software running on some future data format we don't recognize?)
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095077056)
Should we really continue after reading an invalid key length? Feels like it would be good to throw an exception, or `assert` the condition instead of `if`.
(Could adjust the message/comment to include the possibility of us being old software running on some future data format we don't recognize?)
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095101679)
note in b085c52030ec30294f68abfdbe5b05acbae60a7e:
Seems like there's a newish footgun with curly-brace initialization of `vector`s.
This compiles because the passed value unambiguously specifies the length (`std::byte` isn't implicitly constructible from integer literals):
```
std::vector<std::byte> obfuscation{512};
```
The following fails, because it's interpreted as initializer-list construction, and the integer literal would be narrowed to fit into the first element of the `vector`:
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095101679)
note in b085c52030ec30294f68abfdbe5b05acbae60a7e:
Seems like there's a newish footgun with curly-brace initialization of `vector`s.
This compiles because the passed value unambiguously specifies the length (`std::byte` isn't implicitly constructible from integer literals):
```
std::vector<std::byte> obfuscation{512};
```
The following fails, because it's interpreted as initializer-list construction, and the integer literal would be narrowed to fit into the first element of the `vector`:
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095154105)
nit: `0xDEADBEEF` is a 32-bit value. Might be more precise to use a 64-bit literal as above, or randomize?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095154105)
nit: `0xDEADBEEF` is a 32-bit value. Might be more precise to use a 64-bit literal as above, or randomize?
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095188702)
nit: Should we use "old"/"new" qualifiers inline in the code? Feels more appropriate for something in the commit message.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095188702)
nit: Should we use "old"/"new" qualifiers inline in the code? Feels more appropriate for something in the commit message.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095711047)
Thanks for lettering yourself be convinced! Guess I needed another round to sharpen my arguments too.
Nice that you didn't need the `span<uint8_t>` overload.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095711047)
Thanks for lettering yourself be convinced! Guess I needed another round to sharpen my arguments too.
Nice that you didn't need the `span<uint8_t>` overload.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095733067)
Slightly better_better, thanks!
* Why change it from being `private` to `public`, some later PR?
* nit: This compiles on GCC 14.2.1 and Clang 20.1.3:
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index ffb25b8ac1..ffe98a3fde 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -32,6 +32,11 @@
#include <optional>
#include <utility>
+// Prefixed with null character to avoid collisions with other keys
+//
+// We must use a string constructor which specifies len
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095733067)
Slightly better_better, thanks!
* Why change it from being `private` to `public`, some later PR?
* nit: This compiles on GCC 14.2.1 and Clang 20.1.3:
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index ffb25b8ac1..ffe98a3fde 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -32,6 +32,11 @@
#include <optional>
#include <utility>
+// Prefixed with null character to avoid collisions with other keys
+//
+// We must use a string constructor which specifies len
...
💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2891173444)
> ... a version from more than 5 years ago...
6 :)
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2891173444)
> ... a version from more than 5 years ago...
6 :)
💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095802755)
I'd say it would be best to keep all of them. Maybe I am missing the context, but I fail to see how the client version is irrelevant. "they can easily find its version when reporting the bug" doesn't sound great, because some people forget it, and it will just be another round-trip.
Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want
...
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095802755)
I'd say it would be best to keep all of them. Maybe I am missing the context, but I fail to see how the client version is irrelevant. "they can easily find its version when reporting the bug" doesn't sound great, because some people forget it, and it will just be another round-trip.
Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want
...
👍 hebasto approved a pull request: "ci: Move DEBUG=1 to centos task"
(https://github.com/bitcoin/bitcoin/pull/32560#pullrequestreview-2850914158)
ACK fa58d6cdab000df288501db4a71487804b08ba4b.
(https://github.com/bitcoin/bitcoin/pull/32560#pullrequestreview-2850914158)
ACK fa58d6cdab000df288501db4a71487804b08ba4b.
👍 stickies-v approved a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2850770205)
re-ACK fa2445eea3e380386ab01631b735c7eeb6b76b58
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2850770205)
re-ACK fa2445eea3e380386ab01631b735c7eeb6b76b58
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2095810785)
Ah, good point, that seems sensible. (+ for [other](https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093431546))
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2095810785)
Ah, good point, that seems sensible. (+ for [other](https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093431546))
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2095714106)
nit: could assert that uints don't underflow from `-`-prefixed inputs?
<details>
<summary>git diff on fa2445eea3</summary>
```diff
diff --git a/src/test/fuzz/parse_numbers.cpp b/src/test/fuzz/parse_numbers.cpp
index b2465de517..21b1d534f2 100644
--- a/src/test/fuzz/parse_numbers.cpp
+++ b/src/test/fuzz/parse_numbers.cpp
@@ -6,6 +6,9 @@
#include <util/moneystr.h>
#include <util/strencodings.h>
+#include <cassert>
+#include <cstdint>
+#include <optional>
#include <string>
...
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2095714106)
nit: could assert that uints don't underflow from `-`-prefixed inputs?
<details>
<summary>git diff on fa2445eea3</summary>
```diff
diff --git a/src/test/fuzz/parse_numbers.cpp b/src/test/fuzz/parse_numbers.cpp
index b2465de517..21b1d534f2 100644
--- a/src/test/fuzz/parse_numbers.cpp
+++ b/src/test/fuzz/parse_numbers.cpp
@@ -6,6 +6,9 @@
#include <util/moneystr.h>
#include <util/strencodings.h>
+#include <cassert>
+#include <cstdint>
+#include <optional>
#include <string>
...
💬 fanquake commented on pull request "ci: Move DEBUG=1 to centos task":
(https://github.com/bitcoin/bitcoin/pull/32560#discussion_r2095821189)
Any reason to set this to `-g2`, rather than `-g3` (the default for the Debug build type)?.
(https://github.com/bitcoin/bitcoin/pull/32560#discussion_r2095821189)
Any reason to set this to `-g2`, rather than `-g3` (the default for the Debug build type)?.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095821939)
> Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want to quickly reject those, after assessing the bug has nothing to do with any commit in a release or master branch of Bitcoin Core.
I did not think about this yet. I guess that could save a bug report round-trip, or at least help with reducing confusion a bit. Then again, some peopl
...
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095821939)
> Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want to quickly reject those, after assessing the bug has nothing to do with any commit in a release or master branch of Bitcoin Core.
I did not think about this yet. I guess that could save a bug report round-trip, or at least help with reducing confusion a bit. Then again, some peopl
...
💬 laanwj commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2095847319)
From what i could find, this tag covers every version of Windows 10 and 11, not just version 1903+.
(https://github.com/bitcoin/bitcoin/pull/32537#discussion_r2095847319)
From what i could find, this tag covers every version of Windows 10 and 11, not just version 1903+.
👍 ryanofsky approved a pull request: "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2850988140)
Concept ACK bde1fc1289443785e4538c757631d2fa19d5ecd8. Seems like this makes code clearer and faster without changing complexity, and adds tests.
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2850988140)
Concept ACK bde1fc1289443785e4538c757631d2fa19d5ecd8. Seems like this makes code clearer and faster without changing complexity, and adds tests.
⚠️ fanquake opened an issue: "doc: references to unshipped documentation"
(https://github.com/bitcoin/bitcoin/issues/32565)
There are currently multiple places in the `--help` or error output of `bitcoind`, that we reference markdown documentation, that we don't ship with the binaries. i.e:
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L525
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L551
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L1900
https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/issues/32565)
There are currently multiple places in the `--help` or error output of `bitcoind`, that we reference markdown documentation, that we don't ship with the binaries. i.e:
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L525
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L551
https://github.com/bitcoin/bitcoin/blob/ff1ee102c4babadd85e4a18234061f10daaa119d/src/init.cpp#L1900
https://github.com/bitcoin/bit
...
🤔 furszy reviewed a pull request: "wallet: Fix logging of wallet version"
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2851013472)
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2851013472)
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
💬 furszy commented on pull request "wallet: Fix logging of wallet version":
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2891295993)
> With legacy wallets removed, this wouldn't really be possible to test as descriptor wallets all are version 169900.
I was thinking of checking the version during migration, when we load the wallet into memory at the beginning of the process. But np. Small nit.
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2891295993)
> With legacy wallets removed, this wouldn't really be possible to test as descriptor wallets all are version 169900.
I was thinking of checking the version during migration, when we load the wallet into memory at the beginning of the process. But np. Small nit.
💬 hebasto commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2891309997)
> From what I can tell, compilers on Linux systems, will be defining `_GNU_SOURCE`...
Is this behaviour documented? If not, why rely on incidental behaviour that might change?
> ... which results in `glibc` defining `_POSIX_C_SOURCE` to `200809L`...
I don't see any causality here: passing `-U_GNU_SOURCE` to an arbitrary compiler on my machine still results in `_POSIX_C_SOURCE` being set to `200809L`.
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2891309997)
> From what I can tell, compilers on Linux systems, will be defining `_GNU_SOURCE`...
Is this behaviour documented? If not, why rely on incidental behaviour that might change?
> ... which results in `glibc` defining `_POSIX_C_SOURCE` to `200809L`...
I don't see any causality here: passing `-U_GNU_SOURCE` to an arbitrary compiler on my machine still results in `_POSIX_C_SOURCE` being set to `200809L`.