💬 hebasto commented on issue "Control-flow application capabilities for `x86_64-linux-gnu` release binaries":
(https://github.com/bitcoin/bitcoin/issues/30677#issuecomment-2297267192)
Should we adjust glibc [Hardware Capability Tunables](https://www.gnu.org/software/libc/manual/html_node/Hardware-Capability-Tunables.html)?
(https://github.com/bitcoin/bitcoin/issues/30677#issuecomment-2297267192)
Should we adjust glibc [Hardware Capability Tunables](https://www.gnu.org/software/libc/manual/html_node/Hardware-Capability-Tunables.html)?
💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722285129)
you can resolve this comment, I see now that this should definitely be investigated in a separate PR (it's a big change).
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722285129)
you can resolve this comment, I see now that this should definitely be investigated in a separate PR (it's a big change).
💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722286606)
This doesn't sound like it's `GetCoin`'s responsibility, I think the call-sites should be checking for whether the coins are spent or not, not the cache accessor method
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722286606)
This doesn't sound like it's `GetCoin`'s responsibility, I think the call-sites should be checking for whether the coins are spent or not, not the cache accessor method
💬 allanperlee commented on issue "use MiniWallet in functional test `rpc_signrawtransactionwithkey.py` for funding txs":
(https://github.com/bitcoin/bitcoin/issues/30600#issuecomment-2297391140)
@ismaelsadeeq Thanks for reviewing the PR. I corrected the mistake. I really want to get this right, I really appreciate the suggestions.
(https://github.com/bitcoin/bitcoin/issues/30600#issuecomment-2297391140)
@ismaelsadeeq Thanks for reviewing the PR. I corrected the mistake. I really want to get this right, I really appreciate the suggestions.
💬 allanperlee commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2297392127)
@ismaelsadeeq Thanks for reviewing the PR. I corrected the mistake. I really want to get this right, I really appreciate the suggestions.
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2297392127)
@ismaelsadeeq Thanks for reviewing the PR. I corrected the mistake. I really want to get this right, I really appreciate the suggestions.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722313302)
> What about repeated values, which are already using the vector constructor, i.e.
See my message before yours @paplorinc:
> (I think switching to base 10 is or using valtype(36, 0xff); is too inconsistent).
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722313302)
> What about repeated values, which are already using the vector constructor, i.e.
See my message before yours @paplorinc:
> (I think switching to base 10 is or using valtype(36, 0xff); is too inconsistent).
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722229379)
If I change
```C++
std::vector<unsigned char> nonminpush = ParseHex("0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f00ae21ae00100000060606060606000000000000000000000000000000000000000000000000000000000000000000");
const CScript nonminpush_script(nonminpush.begin(), nonminpush.end());
```
->
```C++
const auto nonminpush = HexLiteral<uint8_t>("0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722229379)
If I change
```C++
std::vector<unsigned char> nonminpush = ParseHex("0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f00ae21ae00100000060606060606000000000000000000000000000000000000000000000000000000000000000000");
const CScript nonminpush_script(nonminpush.begin(), nonminpush.end());
```
->
```C++
const auto nonminpush = HexLiteral<uint8_t>("0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721951966)
> It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
It represents indirect use of `ParseHex` with string literals.
> It is incomplete, because it adds a bit of compile-time checking for the new paths using `ToScript`, but leaves the checking out completely for the remaining `ScriptFromHex` paths. It would be better to add the check `data = *Assert(TryParseHex(str))` to `ScriptFromHex` instead and drop this function.
Will add your
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721951966)
> It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
It represents indirect use of `ParseHex` with string literals.
> It is incomplete, because it adds a bit of compile-time checking for the new paths using `ToScript`, but leaves the checking out completely for the remaining `ScriptFromHex` paths. It would be better to add the check `data = *Assert(TryParseHex(str))` to `ScriptFromHex` instead and drop this function.
Will add your
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721852296)
> In my suggestion there is no `std::span`
Yeah, I guess that introduces needless confusion, better to not mention `span`. I like your solution better now. :)
> But I can also imagine a method accepting b.size(), b.begin(), b.end() vs N, b.begin(), b.end(), if you think that's cleaner.
Possibly just `begin()` & `end()` and just using the difference instead of passing `size()` explicitly?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721852296)
> In my suggestion there is no `std::span`
Yeah, I guess that introduces needless confusion, better to not mention `span`. I like your solution better now. :)
> But I can also imagine a method accepting b.size(), b.begin(), b.end() vs N, b.begin(), b.end(), if you think that's cleaner.
Possibly just `begin()` & `end()` and just using the difference instead of passing `size()` explicitly?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297409003)
@maflcko:
> If you decide to keep the last two commits, it would be good to correct the scripted-diff, because I think it is wrong and just happens to work by accident. The replacement is `HexLiteral\1<uint8_t>`, where `\1` refers to the original inner Byte type, for example `<std::byte>`. However `HexLiteral<std::byte><uint8_t>` wouldn't be valid C++ code, when the scripted-diff happens to pick it up in the future.
Well spotted! I wrote it that way for it to fail compilation and prompt manu
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297409003)
@maflcko:
> If you decide to keep the last two commits, it would be good to correct the scripted-diff, because I think it is wrong and just happens to work by accident. The replacement is `HexLiteral\1<uint8_t>`, where `\1` refers to the original inner Byte type, for example `<std::byte>`. However `HexLiteral<std::byte><uint8_t>` wouldn't be valid C++ code, when the scripted-diff happens to pick it up in the future.
Well spotted! I wrote it that way for it to fail compilation and prompt manu
...
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722325152)
`valtype` is not the point here, rather that for repeated values we've used the vector constructor.
But we can close this comment, maybe we'll do it in another PR.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722325152)
`valtype` is not the point here, rather that for repeated values we've used the vector constructor.
But we can close this comment, maybe we'll do it in another PR.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803)
I'm fine with any of those - having a slight preference for the one which clarifies the two separate roles of the serialization - length + values
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803)
I'm fine with any of those - having a slight preference for the one which clarifies the two separate roles of the serialization - length + values
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722329648)
Yes. `valtype` was not my only point, it was also the repeated values.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722329648)
Yes. `valtype` was not my only point, it was also the repeated values.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722332708)
But aren't we already doing that throughout the test, e.g. https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L1031?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722332708)
But aren't we already doing that throughout the test, e.g. https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L1031?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722336406)
Yes, we are. But I'd rather not change that aspect of this region in this PR. Arguably the current "flattened out" version is clearer for the casual reader who isn't interested in the exact number of bytes being repeated.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722336406)
Yes, we are. But I'd rather not change that aspect of this region in this PR. Arguably the current "flattened out" version is clearer for the casual reader who isn't interested in the exact number of bytes being repeated.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722338360)
Ah, so it's just out of scope - I can work with that. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722338360)
Ah, so it's just out of scope - I can work with that. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297461028)
The major change in the latest push is that I've broken out the changes to `CCrypter` + tests into several more commits (code improvement, de-Hungarianization rename, vector->span, separate XOnlyPubKey) to try to clarify what is actually happening. Could have sworn someone suggested it but having trouble finding the comment.
I've implemented `ScriptFromHex` in terms of a modified `ToScript` in a last attempt before [maflcko gets to veto it](https://github.com/bitcoin/bitcoin/pull/30377#discus
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297461028)
The major change in the latest push is that I've broken out the changes to `CCrypter` + tests into several more commits (code improvement, de-Hungarianization rename, vector->span, separate XOnlyPubKey) to try to clarify what is actually happening. Could have sworn someone suggested it but having trouble finding the comment.
I've implemented `ScriptFromHex` in terms of a modified `ToScript` in a last attempt before [maflcko gets to veto it](https://github.com/bitcoin/bitcoin/pull/30377#discus
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722354071)
Unless someone finds a way to maintain symmetry using ranges or something, I'm keeping as-is.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722354071)
Unless someone finds a way to maintain symmetry using ranges or something, I'm keeping as-is.
💬 tdb3 commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1722354614)
Thanks for reviewing. Implemented
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1722354614)
Thanks for reviewing. Implemented
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722356732)
Settled for fixing this in 41d97d38bfbdacdf56e730c6b082f992e4f15ec1, before the scripted-diff happens.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722356732)
Settled for fixing this in 41d97d38bfbdacdf56e730c6b082f992e4f15ec1, before the scripted-diff happens.