💬 Sjors commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2865947567)
> If desired, tests could be made.
I don't understand annexes at all, so having some tests to illustrate the issue would be handy.
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2865947567)
> If desired, tests could be made.
I don't understand annexes at all, so having some tests to illustrate the issue would be handy.
🤔 l0rinc requested changes to a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2827771860)
I happened to be working on a PR that's related to the exact same code path.
I'm not sure whether the call is redundant or not, but I can understand why we want to avoid situations like https://github.com/bitcoin/bitcoin/pull/9049.
I would prefer a more high-level explanation for the difference between the two calls - explained through code and tests preferably instead of comments - or maybe both.
There are a few minor typos in the commit message as well and it's a bit hand-wavy - can w
...
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2827771860)
I happened to be working on a PR that's related to the exact same code path.
I'm not sure whether the call is redundant or not, but I can understand why we want to avoid situations like https://github.com/bitcoin/bitcoin/pull/9049.
I would prefer a more high-level explanation for the difference between the two calls - explained through code and tests preferably instead of comments - or maybe both.
There are a few minor typos in the commit message as well and it's a bit hand-wavy - can w
...
💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081363131)
> it does not count witness and p2sh sigops
Isn't it obvious from the name `Get*Legacy*SigOpCount` and the parameter `GetSigOpCount(bool fAccurate)` called as `GetSigOpCount(false)` that new script types aren't covered and that it's not meant to be accurate but rather compatible with old behavior?
Instead (or maybe next to) the comments, I'd find it more helpful if we documented what the `false` means in `GetLegacySigOpCount`:
```C++
unsigned int GetLegacySigOpCount(const CTransaction& t
...
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081363131)
> it does not count witness and p2sh sigops
Isn't it obvious from the name `Get*Legacy*SigOpCount` and the parameter `GetSigOpCount(bool fAccurate)` called as `GetSigOpCount(false)` that new script types aren't covered and that it's not meant to be accurate but rather compatible with old behavior?
Instead (or maybe next to) the comments, I'd find it more helpful if we documented what the `false` means in `GetLegacySigOpCount`:
```C++
unsigned int GetLegacySigOpCount(const CTransaction& t
...
💬 Sjors commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2865986579)
> as long as they let it completely finish once before starting bitcoind
That seems likely to cause issues. It seems better if we track the height at which xor starts. A script could then work backwards and lower the height. And in that case it might as well be an RPC command.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2865986579)
> as long as they let it completely finish once before starting bitcoind
That seems likely to cause issues. It seems better if we track the height at which xor starts. A script could then work backwards and lower the height. And in that case it might as well be an RPC command.
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081395687)
> Isn't it obvious from the name
Given our history of bad function names, no :-)
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081395687)
> Isn't it obvious from the name
Given our history of bad function names, no :-)
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2866037471)
The purpose of this comment isn't to be throrough, it's there to:
1. Warn callers to not rely _only_ on this check
2. For anyone who wants to get rid of this check, point to the relevant context
I think (2) can be achieved with just a commit message and the fact that our merge script points back to the original PR.
I'm not opposed to writing a longer comment, but I suspect it would take a long time to find a text everyone agrees on. And by the time we do, we might as well change the co
...
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2866037471)
The purpose of this comment isn't to be throrough, it's there to:
1. Warn callers to not rely _only_ on this check
2. For anyone who wants to get rid of this check, point to the relevant context
I think (2) can be achieved with just a commit message and the fact that our merge script points back to the original PR.
I'm not opposed to writing a longer comment, but I suspect it would take a long time to find a text everyone agrees on. And by the time we do, we might as well change the co
...
💬 hebasto commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866040856)
> When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release.
If this matters, the following patch could help:
```diff
--- a/.gitattributes
+++ b/.gitattributes
@@ -1 +1,2 @@
+CMakeLists.txt export-subst
src/clientversion.cpp export-subst
diff --git a/CMakeLists.txt b/CMakeLists.txt
index a1665563cb..d0d24db713 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,6 +29,7 @@ set(CLIENT_VERSION_BUILD 0)
set
...
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866040856)
> When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release.
If this matters, the following patch could help:
```diff
--- a/.gitattributes
+++ b/.gitattributes
@@ -1 +1,2 @@
+CMakeLists.txt export-subst
src/clientversion.cpp export-subst
diff --git a/CMakeLists.txt b/CMakeLists.txt
index a1665563cb..d0d24db713 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,6 +29,7 @@ set(CLIENT_VERSION_BUILD 0)
set
...
💬 laanwj commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2866068613)
> It seems better if we track the height at which xor starts
i really like this idea. "Start XORing from now on" as a default would be enough to prevent new problems, and potentially there could be a background process that updates historical blocks (which would be way less performance critical as no one is waiting for it).
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2866068613)
> It seems better if we track the height at which xor starts
i really like this idea. "Start XORing from now on" as a default would be enough to prevent new problems, and potentially there could be a background process that updates historical blocks (which would be way less performance critical as no one is waiting for it).
💬 Sjors commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2866112061)
Concept ACK. 2d8679c45107598689cc2af90e6324fa9172b6d3 looks reasonable, though linter is unhappy.
Maybe move b4f5b4e37d29c1a516186775736efda9d77c2fa9 "test: Remove unnecessary importprivkey from wallet_createwallet" to the top and then combine "Add wallet_importprivkey helper for replacing importprivkey" with the commit that replaces the usage. And then also drop `importprivkey` from the `RPCOverloadWrapper`.
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2866112061)
Concept ACK. 2d8679c45107598689cc2af90e6324fa9172b6d3 looks reasonable, though linter is unhappy.
Maybe move b4f5b4e37d29c1a516186775736efda9d77c2fa9 "test: Remove unnecessary importprivkey from wallet_createwallet" to the top and then combine "Add wallet_importprivkey helper for replacing importprivkey" with the commit that replaces the usage. And then also drop `importprivkey` from the `RPCOverloadWrapper`.
👍 rkrux approved a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2827881979)
tACK 1ee698fde2e8652d8eb083749edb8029c003b8db
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2827881979)
tACK 1ee698fde2e8652d8eb083749edb8029c003b8db
💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081429882)
Given that `s[0] == 0` is asserted above and the signature is negated from `s[1]` onwards, I was trying to reason why this check is still needed before dropping `s[0]` & thought of the reason that if `s[0]` is dropped when `s[1] >= 0x80` then `s` will be treated as a negative number, which is illegal as mentioned in https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574.
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081429882)
Given that `s[0] == 0` is asserted above and the signature is negated from `s[1]` onwards, I was trying to reason why this check is still needed before dropping `s[0]` & thought of the reason that if `s[0]` is dropped when `s[1] >= 0x80` then `s` will be treated as a negative number, which is illegal as mentioned in https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574.
💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081451063)
I checked that `ret` here means whether the passed `signature/seckey` in the second argument is valid according to `secp256k1_ec_seckey_verify`, which weirdly enough doesn't seem to be called by `secp256k1_ec_seckey_negate` - maybe the doc is outdated, but I get the general idea of validating the input & returning the response of this check.
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/secp256k1/include/secp256k1.h#L689-L702
https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081451063)
I checked that `ret` here means whether the passed `signature/seckey` in the second argument is valid according to `secp256k1_ec_seckey_verify`, which weirdly enough doesn't seem to be called by `secp256k1_ec_seckey_negate` - maybe the doc is outdated, but I get the general idea of validating the input & returning the response of this check.
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/secp256k1/include/secp256k1.h#L689-L702
https://github.com/bitcoi
...
💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866148542)
> Not sure if this is worth it. Currently 4 lines of code need to be changed, looking at https://github.com/bitcoin/bitcoin/commit/b537a2c02a9921235d1ecf8c3c7dc1836ec68131. After this, 3 lines will need to be changed. I don't really see the benefit here.
Yeah, i more or less agree. It's already been condensed to one place, maybe that's enough.
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866148542)
> Not sure if this is worth it. Currently 4 lines of code need to be changed, looking at https://github.com/bitcoin/bitcoin/commit/b537a2c02a9921235d1ecf8c3c7dc1836ec68131. After this, 3 lines will need to be changed. I don't really see the benefit here.
Yeah, i more or less agree. It's already been condensed to one place, maybe that's enough.
💬 laanwj commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081461596)
Right.
It'd be possible to structure this slightly more intuitive, by first removing the first byte (after asserting it's 0x00), then doing `secp256_blabla` on the entire 32-byte thing, then conditionally readding it after.
But meh. It's not like this is a easty-to-digest function in the first place 😓
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081461596)
Right.
It'd be possible to structure this slightly more intuitive, by first removing the first byte (after asserting it's 0x00), then doing `secp256_blabla` on the entire 32-byte thing, then conditionally readding it after.
But meh. It's not like this is a easty-to-digest function in the first place 😓
✅ fanquake closed a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)
(https://github.com/bitcoin/bitcoin/pull/32445)
💬 laanwj commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2866165846)
> The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
i think this makes sense, especially for the symbol check. The security check may have some value as a more general "binary security check". But moving both to a guix-specific path would be fine with me.
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2866165846)
> The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.
i think this makes sense, especially for the symbol check. The security check may have some value as a more general "binary security check". But moving both to a guix-specific path would be fine with me.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476373)
I admit defeat, added:
```C++
/** Generate random bytes on the stack. */
template <BasicByte B = unsigned char, size_t N>
std::array<B, N> randbytes() noexcept
{
std::array<B, N> ret;
Impl().fillrand(MakeWritableByteSpan(ret));
return ret;
}
```
See: a66c1841bd5708eb446c805d6612e2448d6022f3
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476373)
I admit defeat, added:
```C++
/** Generate random bytes on the stack. */
template <BasicByte B = unsigned char, size_t N>
std::array<B, N> randbytes() noexcept
{
std::array<B, N> ret;
Impl().fillrand(MakeWritableByteSpan(ret));
return ret;
}
```
See: a66c1841bd5708eb446c805d6612e2448d6022f3
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476663)
Some compilers had problem with string `constexpr`, it's why I inlined it instead.
I don't mind reverting to `OBFUSCATION_KEY_KEY` (note `OBFUSCATE` -> `OBFUSCATION`), if you think it's better_better.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476663)
Some compilers had problem with string `constexpr`, it's why I inlined it instead.
I don't mind reverting to `OBFUSCATION_KEY_KEY` (note `OBFUSCATE` -> `OBFUSCATION`), if you think it's better_better.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081477824)
Thanks
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081477824)
Thanks
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2866204024)
There was a seemingly intermittent CI failure in wallet_basic.py on the commit merging this into master. Tracked as #32456. The following push to master succeeded.
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2866204024)
There was a seemingly intermittent CI failure in wallet_basic.py on the commit merging this into master. Tracked as #32456. The following push to master succeeded.