💬 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.
💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081485161)
Thanks for clarifying. I got the impression that you did think it was useful from "It may be a good idea to actually improve the check here, since it's relied on by `getblocktemplate` in proposal mode and by #31981" (https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2053597806). But I guess you were saying it *could* be useful if it were improved, not that it is useful currently.
Anyway I was mostly asking about this because I was confused. Assuming you are happy with PR in current fo
...
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081485161)
Thanks for clarifying. I got the impression that you did think it was useful from "It may be a good idea to actually improve the check here, since it's relied on by `getblocktemplate` in proposal mode and by #31981" (https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2053597806). But I guess you were saying it *could* be useful if it were improved, not that it is useful currently.
Anyway I was mostly asking about this because I was confused. Assuming you are happy with PR in current fo
...
💬 carnhofdaki commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2866215127)
Concept ACK, the patches look fine to me. Compiling 92b05c17b1 right now. Thanks for doing it!
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2866215127)
Concept ACK, the patches look fine to me. Compiling 92b05c17b1 right now. Thanks for doing it!
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2081493334)
Good catch! I do think daemon could be ok but I agree node is better. I will update this (and I also want to look into @theStack's suggestion to use subprocess.h, which I was unaware of and could simplify the PR)
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2081493334)
Good catch! I do think daemon could be ok but I agree node is better. I will update this (and I also want to look into @theStack's suggestion to use subprocess.h, which I was unaware of and could simplify the PR)