💬 fjahr commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2890959492)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2890959492)
Concept ACK
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2095672887)
No, because when indented then it is rendered as a code block.
OK:
---
## 1. Run Bitcoin Core behind a Tor proxy
The first step is running Bitcoin Core behind a Tor proxy. This will already anonymize all
outgoing connections, but more is possible.
-proxy=ip[:port]
Set the proxy server. It will be used to try to reach .onion addresses
as well. You need to use -noonion or -onion=0 to explicitly disable
outbound access to onion services.
-proxy=ip
...
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2095672887)
No, because when indented then it is rendered as a code block.
OK:
---
## 1. Run Bitcoin Core behind a Tor proxy
The first step is running Bitcoin Core behind a Tor proxy. This will already anonymize all
outgoing connections, but more is possible.
-proxy=ip[:port]
Set the proxy server. It will be used to try to reach .onion addresses
as well. You need to use -noonion or -onion=0 to explicitly disable
outbound access to onion services.
-proxy=ip
...
💬 fjahr commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2890980318)
Concept ACK
I agree with the rationale, will test it shortly.
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2890980318)
Concept ACK
I agree with the rationale, will test it shortly.
🤔 danielabrozzoni reviewed a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2850728936)
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
I'm not too familiar with the signet miner, but the code looks good to me. I ran the tests locally, and did some manual testing using `-signetchallenge=51` and checking that the signet commitment is absent.
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2850728936)
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
I'm not too familiar with the signet miner, but the code looks good to me. I ran the tests locally, and did some manual testing using `-signetchallenge=51` and checking that the signet commitment is absent.
💬 pinheadmz commented on pull request "[28.x] Backport #31407":
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2891009384)
Concept ACK, starting guix build of this branch and will try to codesign with certificate
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2891009384)
Concept ACK, starting guix build of this branch and will try to codesign with certificate
👍 willcl-ark approved a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2850764439)
ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2850764439)
ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db
💬 hebasto commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353)
> I don't think so; MSVC is _only_ complaining about consteval literals inside `( ? : )` ternaries. I've pushed another attempt to avoid it by moving the subexpressions out of the ternaries.
FWIW, this bug was fixed in VS 17.9.
See https://developercommunity.visualstudio.com/t/Ternary-operator-breaks-consteval/10150913.
(https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353)
> I don't think so; MSVC is _only_ complaining about consteval literals inside `( ? : )` ternaries. I've pushed another attempt to avoid it by moving the subexpressions out of the ternaries.
FWIW, this bug was fixed in VS 17.9.
See https://developercommunity.visualstudio.com/t/Ternary-operator-breaks-consteval/10150913.
💬 fanquake commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2891088686)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2891088686)
Concept ACK
📝 hebasto opened a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564)
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.
The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.
(https://github.com/bitcoin/bitcoin/pull/32564)
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.
The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095777325)
> forgot to remove the include?
yes -_-
> However, the CLIENT_NAME symbol is still used in the kernel,
Mmh, that is done though the config header, and I am not sure how we should handle that yet. Seems like a separate problem to me? Then again the clientversion header is little more than some formatting helpers for some these variables. Could also remove `CLIENT_NAME` in the few cases where it is relevant, but `CLIENT_BUGREPORT` is probably useful to keep.
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095777325)
> forgot to remove the include?
yes -_-
> However, the CLIENT_NAME symbol is still used in the kernel,
Mmh, that is done though the config header, and I am not sure how we should handle that yet. Seems like a separate problem to me? Then again the clientversion header is little more than some formatting helpers for some these variables. Could also remove `CLIENT_NAME` in the few cases where it is relevant, but `CLIENT_BUGREPORT` is probably useful to keep.
💬 hebasto commented on pull request "cmake: Add missed `SSE41_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2891147706)
> Just roll this into #32551?
Done.
(https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2891147706)
> Just roll this into #32551?
Done.
✅ hebasto closed a pull request: "cmake: Add missed `SSE41_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/32550)
(https://github.com/bitcoin/bitcoin/pull/32550)
💬 hebasto commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2891148659)
Add a commit from https://github.com/bitcoin/bitcoin/pull/32550.
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2891148659)
Add a commit from https://github.com/bitcoin/bitcoin/pull/32550.
🤔 hodlinator reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2849775833)
Code review 9406c17223da9df3a9875e4a5769a96ae66d61b8
Getting pretty close to ACK. One main inlined question about error handling in dbwrapper.cpp.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2849775833)
Code review 9406c17223da9df3a9875e4a5769a96ae66d61b8
Getting pretty close to ACK. One main inlined question about error handling in dbwrapper.cpp.
💬 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
...