💬 fjahr commented on pull request "doc: mention bip94 support":
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2290950882)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2290950882)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
🚀 glozow merged a pull request: "doc: mention bip94 support"
(https://github.com/bitcoin/bitcoin/pull/30655)
(https://github.com/bitcoin/bitcoin/pull/30655)
👍 TheCharlatan approved a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2240096761)
ACK d438b501f859befa4f758efcff463c0d647bc033
The important change here is that when used with cmake, the user-defined flags currently get placed in the toolchain file like this (running `make CXXFLAGS="-O1" `):
```
32 if(NOT DEFINED CMAKE_CXX_FLAGS_INIT)
33 set(CMAKE_CXX_FLAGS_INIT "")
34 set(CMAKE_OBJCXX_FLAGS_INIT "")
35 endif()
36 if(NOT DEFINED CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT)
37 set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "-O1")
38 set(CMAKE_OBJCXX_FLAGS_RELWITHDEB
...
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2240096761)
ACK d438b501f859befa4f758efcff463c0d647bc033
The important change here is that when used with cmake, the user-defined flags currently get placed in the toolchain file like this (running `make CXXFLAGS="-O1" `):
```
32 if(NOT DEFINED CMAKE_CXX_FLAGS_INIT)
33 set(CMAKE_CXX_FLAGS_INIT "")
34 set(CMAKE_OBJCXX_FLAGS_INIT "")
35 endif()
36 if(NOT DEFINED CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT)
37 set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "-O1")
38 set(CMAKE_OBJCXX_FLAGS_RELWITHDEB
...
🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239975805)
Left some style nits, feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239975805)
Left some style nits, feel free to ignore.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718187725)
style nit in https://github.com/bitcoin/bitcoin/commit/b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795:
I wonder if it makes sense to pick the default as `Byte = std::byte` now. Obviously the diff will be larger, but longer term it seems cleaner to converge to `std::byte`, so having that as default will make the future easier, no?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718187725)
style nit in https://github.com/bitcoin/bitcoin/commit/b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795:
I wonder if it makes sense to pick the default as `Byte = std::byte` now. Obviously the diff will be larger, but longer term it seems cleaner to converge to `std::byte`, so having that as default will make the future easier, no?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718137078)
style nit in 9c4aad3fe4cb74bb1d9afeece78e2dd6ae1ad08e: When renaming this, I'd suggest to use the "fully" correct naming style `ciphertext` (snake_case without type prefix), according to the style guide.
(An alternative would be to leave it completely untouched)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718137078)
style nit in 9c4aad3fe4cb74bb1d9afeece78e2dd6ae1ad08e: When renaming this, I'd suggest to use the "fully" correct naming style `ciphertext` (snake_case without type prefix), according to the style guide.
(An alternative would be to leave it completely untouched)
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718210663)
style nit in 0a82c18457ec81e911b835b2ac76ad7475384983:
I know this has been mentioned before, but performance doesn't matter here, so using just `VecFromHex` seems shorter and clearer?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718210663)
style nit in 0a82c18457ec81e911b835b2ac76ad7475384983:
I know this has been mentioned before, but performance doesn't matter here, so using just `VecFromHex` seems shorter and clearer?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718211451)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Span and array serialization is the same, so in theory you could drop this call to Span.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718211451)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Span and array serialization is the same, so in theory you could drop this call to Span.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718214663)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.
The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718214663)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:
Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.
The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than
...
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718237308)
That's strange, I don't see how a string literal would need a conversion when there's a `const char*` ctor? It seems to work fine for [`ConstevalStringLiteral`](https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/util/translation.h#L72) too, so pardon my insistence but I think perhaps this compiler error might be because of another issue? Would you mind trying this diff on 734ac5a9002493013c0f8afe763f751ac99f89c8?
<details>
<summary>git diff on 734ac5a900</su
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718237308)
That's strange, I don't see how a string literal would need a conversion when there's a `const char*` ctor? It seems to work fine for [`ConstevalStringLiteral`](https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/util/translation.h#L72) too, so pardon my insistence but I think perhaps this compiler error might be because of another issue? Would you mind trying this diff on 734ac5a9002493013c0f8afe763f751ac99f89c8?
<details>
<summary>git diff on 734ac5a900</su
...
💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718238341)
I agree. Would either go all-in, or not do the rename at all.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718238341)
I agree. Would either go all-in, or not do the rename at all.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718218266)
nit: this is closely tied to `ParseHex_expected`, if you modify again, consider extracting all 5 usages next to it.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718218266)
nit: this is closely tied to `ParseHex_expected`, if you modify again, consider extracting all 5 usages next to it.
🤔 paplorinc reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240096017)
We're getting closer, I still hope we don't have to sacrifice some readability.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240096017)
We're getting closer, I still hope we don't have to sacrifice some readability.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718251992)
If we insist that this is an important check, it seems a bit jumpy to do the check at the beginning, after which we iterate over the elements and arrive exactly at that element - and just silently skip it.
Alternatively, we could check for `\0` when we get there, after we've processed the rest of the chars, i.e.
```C++
assert(!hex_str[i]);
return rv;
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718251992)
If we insist that this is an important check, it seems a bit jumpy to do the check at the beginning, after which we iterate over the elements and arrive exactly at that element - and just silently skip it.
Alternatively, we could check for `\0` when we get there, after we've processed the rest of the chars, i.e.
```C++
assert(!hex_str[i]);
return rv;
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718241896)
This was quite simple before, now it might be faster by a few nanos, but it's less readable.
Given that `HexStr` was already tested properly, could we maybe go the other way and convert `detsig` to hex string and compare against the hard coded value, i.e.
```suggestion
BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718241896)
This was quite simple before, now it might be faster by a few nanos, but it's less readable.
Given that `HexStr` was already tested properly, could we maybe go the other way and convert `detsig` to hex string and compare against the hard coded value, i.e.
```suggestion
BOOST_CHECK_EQUAL(HexStr(detsig), "304402205dbbddda71772d95ce91cd2d14b592cfbc1dd0aabd6a394b6c2d377bbe59d31d022014ddda21494a4e221f0824f0b8b924c43fa43c0ad57dccdaa11f81a6bd4582f6");
```
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718252997)
btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the `.run(` part, so I'd go with the cleanest code here, too.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718252997)
btw, this is in the preprocessing phase of the benchmark, the performance doesn't matter here at all - only inside the `.run(` part, so I'd go with the cleanest code here, too.
💬 sipa commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2291069514)
@vasild Yes, but that gratuitously reveals to its peers that it is the transaction originator too (not really an issue in your private broadcast setting, but in nearly all others, this would be undesirable for that reason).
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2291069514)
@vasild Yes, but that gratuitously reveals to its peers that it is the transaction originator too (not really an issue in your private broadcast setting, but in nearly all others, this would be undesirable for that reason).
💬 twofaktor commented on issue "[FEATURE REQUEST] Enable new Tor PoW feature for automatic creation of Bitcoin Core onion hidden service":
(https://github.com/bitcoin/bitcoin/issues/28499#issuecomment-2291079391)
> @twofaktor if you'd like me to open a PR with this doc update, then let me know and I'd be happy to do that.
Hi, thanks for your dedication, IMO, if it is possible to add this secure protection feature in any of the variety of configuration cases (manually, without using the port control method), I think it should be added to the docs. If at some point the possibility of using it also using the port control arrives, add it when the time comes
(https://github.com/bitcoin/bitcoin/issues/28499#issuecomment-2291079391)
> @twofaktor if you'd like me to open a PR with this doc update, then let me know and I'd be happy to do that.
Hi, thanks for your dedication, IMO, if it is possible to add this secure protection feature in any of the variety of configuration cases (manually, without using the port control method), I think it should be added to the docs. If at some point the possibility of using it also using the port control arrives, add it when the time comes
💬 tdb3 commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2291090303)
> > Saw some `may be used uninitialized` warnings when building, e.g.
>
> What compiler? I don't see those warnings.
gcc/g++ (Debian 12.2.0-14) 12.2.0
Looks like it's on my end. Did a fresh clone and build and didn't see the same warnings.
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2291090303)
> > Saw some `may be used uninitialized` warnings when building, e.g.
>
> What compiler? I don't see those warnings.
gcc/g++ (Debian 12.2.0-14) 12.2.0
Looks like it's on my end. Did a fresh clone and build and didn't see the same warnings.
👍 tdb3 approved a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2240178506)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2240178506)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2