💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721364492)
Will do if I retouch.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721364492)
Will do if I retouch.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720779733)
As I said here: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131
> I tried experimenting with user defined literals in response now but ran into issues with both making them `consteval` and accepting a `size_t`-templated char-array argument.
If we are to return a `std::array` I think we need to take the size as a template argument I think. And no compiler seems to accept `consteval` user defined literals.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720779733)
As I said here: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131
> I tried experimenting with user defined literals in response now but ran into issues with both making them `consteval` and accepting a `size_t`-templated char-array argument.
If we are to return a `std::array` I think we need to take the size as a template argument I think. And no compiler seems to accept `consteval` user defined literals.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295942259)
Added section on `std::array<std::byte>` to PR summary.
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295942259)
Added section on `std::array<std::byte>` to PR summary.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721392784)
> once we have full std::array<std::byte> support in CSscript:
Valid point.
----
What about repeated values, which are already using the vector constructor, i.e.
```C++
valtype(36, 0xff)
```
instead of
```C++
Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721392784)
> once we have full std::array<std::byte> support in CSscript:
Valid point.
----
What about repeated values, which are already using the vector constructor, i.e.
```C++
valtype(36, 0xff)
```
instead of
```C++
Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
```
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295963942)
I haven't reviewed the last two commits, because I think the test-only changes offers the least amount of benefits, while being the hardest to review, because the type is changed and thus one has to make sure the call graph is still the same.
I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function). Otherwise, the tests will be changed again a
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295963942)
I haven't reviewed the last two commits, because I think the test-only changes offers the least amount of benefits, while being the hardest to review, because the type is changed and thus one has to make sure the call graph is still the same.
I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function). Otherwise, the tests will be changed again a
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721377340)
nit in 309d8b783109eb52a5596712b19210271c8f882e (and the next commit):
I know I've raised this before in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866, but I still think it would be cleaner to drop this function (and related changes) from this pull request, because:
* It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
* It is incomplete, because it adds a bit of compile-time checking for the new paths using `
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721377340)
nit in 309d8b783109eb52a5596712b19210271c8f882e (and the next commit):
I know I've raised this before in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866, but I still think it would be cleaner to drop this function (and related changes) from this pull request, because:
* It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
* It is incomplete, because it adds a bit of compile-time checking for the new paths using `
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721346102)
9c8d091382909f44c2fc61b8c726362356db2bff: Why is vec needed in this file?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721346102)
9c8d091382909f44c2fc61b8c726362356db2bff: Why is vec needed in this file?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721412277)
> What about repeated values, which are already using the vector constructor, i.e.
I'd say that there is more value in a test being consistent (using the same code patterns within a single unit test), than to use the shortest possible code. However that is a style question up to the author. Personally, I'd leave this line in this pull request as-is.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721412277)
> What about repeated values, which are already using the vector constructor, i.e.
I'd say that there is more value in a test being consistent (using the same code patterns within a single unit test), than to use the shortest possible code. However that is a style question up to the author. Personally, I'd leave this line in this pull request as-is.
⚠️ Sjors opened an issue: "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with 18.1.3"
(https://github.com/bitcoin/bitcoin/issues/30674)
The Cirrus CI on my fork of the repo runs on Ubuntu 24.04 with kernel version 6.8.0-38. This has `vm.mmap_rnd_bits=32` set, which causes the TSAN and MSAN jobs to fail.
See:
TSAN: https://cirrus-ci.com/task/6619444124844032
```
FAIL: minisketch/test
=====================
ThreadSanitizer: CHECK failed: tsan_platform_linux.cpp:282 "((personality(old_personality | ADDR_NO_RANDOMIZE))) != ((-1))" (0xffffffffffffffff, 0xffffffffffffffff) (tid=42931)
FAIL minisketch/test (exit status: 139
...
(https://github.com/bitcoin/bitcoin/issues/30674)
The Cirrus CI on my fork of the repo runs on Ubuntu 24.04 with kernel version 6.8.0-38. This has `vm.mmap_rnd_bits=32` set, which causes the TSAN and MSAN jobs to fail.
See:
TSAN: https://cirrus-ci.com/task/6619444124844032
```
FAIL: minisketch/test
=====================
ThreadSanitizer: CHECK failed: tsan_platform_linux.cpp:282 "((personality(old_personality | ADDR_NO_RANDOMIZE))) != ((-1))" (0xffffffffffffffff, 0xffffffffffffffff) (tid=42931)
FAIL minisketch/test (exit status: 139
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721414782)
I agree with Ryan, that this should be a follow-up PR, and that the changes in this line of the diff should probably be kept as-is.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721414782)
I agree with Ryan, that this should be a follow-up PR, and that the changes in this line of the diff should probably be kept as-is.
💬 ismaelsadeeq commented on issue "wallet: setting changes are subject to race conditions":
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2296019290)
Yes, I encountered the race condition after running the test with the diff you provided.
I believe I fixed the race condition by locking the wallet context mutex in both `AddWalletSettings` and `RemoveWalletSettings`.
However, the test will still fail because the loop variable `i` is captured by reference. This means that all threads share the same reference to `i`, leading to situations where multiple threads might update the same wallet name. As a result, the size of the settings can b
...
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2296019290)
Yes, I encountered the race condition after running the test with the diff you provided.
I believe I fixed the race condition by locking the wallet context mutex in both `AddWalletSettings` and `RemoveWalletSettings`.
However, the test will still fail because the loop variable `i` is captured by reference. This means that all threads share the same reference to `i`, leading to situations where multiple threads might update the same wallet name. As a result, the size of the settings can b
...
🤔 ismaelsadeeq reviewed a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2244970458)
re-ACK eff5bf7d673c797d63c5ad15ac18b8316dea5ffe
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2244970458)
re-ACK eff5bf7d673c797d63c5ad15ac18b8316dea5ffe
👍 vasild approved a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2244976481)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
I did not review this entirely and thoroughly like I do for other pull requests. The development that lead to this happened in https://github.com/hebasto/bitcoin/pulls where every change that is in this PR was reviewed by me and/or other reviewers. In this respect, this is similar to `master` where I did not review all changes, but it is possible to check that all changes were properly reviewed (by me and/or other reviewers).
This PR probably ha
...
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2244976481)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
I did not review this entirely and thoroughly like I do for other pull requests. The development that lead to this happened in https://github.com/hebasto/bitcoin/pulls where every change that is in this PR was reviewed by me and/or other reviewers. In this respect, this is similar to `master` where I did not review all changes, but it is possible to check that all changes were properly reviewed (by me and/or other reviewers).
This PR probably ha
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2296047020)
Still haven't reviewed the last two commits, otherwise the changes since my previous review were:
* Add back the comment to the `XOnlyPubKey` constructor (taking a span)
* Use safer `UCharCast` in one instance
* Some refactoring in the `crypter` module (Personally, I'd prefer if multi-line refactoring were done in a separate commit, not mixed with renaming and type-changes, but not sure if it makes sense to change this pull now)
re-ACK 67fc994bedf14e360b3e51fa1a71dc6c1684b532 🍠
<deta
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2296047020)
Still haven't reviewed the last two commits, otherwise the changes since my previous review were:
* Add back the comment to the `XOnlyPubKey` constructor (taking a span)
* Use safer `UCharCast` in one instance
* Some refactoring in the `crypter` module (Personally, I'd prefer if multi-line refactoring were done in a separate commit, not mixed with renaming and type-changes, but not sure if it makes sense to change this pull now)
re-ACK 67fc994bedf14e360b3e51fa1a71dc6c1684b532 🍠
<deta
...
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721451687)
I haven't seen https://github.com/bitcoin/bitcoin/commit/5be34598c4683b2b44f607b28592a9e68e089761
It seems to me there's a simpler solution than that, since the method contains two separate concerns (inserting size + inserting elements) - which are using different parts of the vector/array.
```patch
diff --git a/src/script/script.h b/src/script/script.h
--- a/src/script/script.h (revision d5eed066d33780b12c1f0813a2adf021eac0ca5d)
+++ b/src/script/script.h (date 1724057506103)
@@ -412,6 +41
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721451687)
I haven't seen https://github.com/bitcoin/bitcoin/commit/5be34598c4683b2b44f607b28592a9e68e089761
It seems to me there's a simpler solution than that, since the method contains two separate concerns (inserting size + inserting elements) - which are using different parts of the vector/array.
```patch
diff --git a/src/script/script.h b/src/script/script.h
--- a/src/script/script.h (revision d5eed066d33780b12c1f0813a2adf021eac0ca5d)
+++ b/src/script/script.h (date 1724057506103)
@@ -412,6 +41
...
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721460986)
I'm not advocating for it, but I think we can get it to compile by something like:
```C++
consteval auto operator"" _hex(const char* hex_str, size_t _)
{
constexpr std::size_t len = sizeof(hex_str);
static_assert(len % 2 == 0, "Hex string must have an even number of characters");
if (hex_str[len - 1] != '\0') throw "null terminator required";
std::array<std::byte, len / 2> rv{};
size_t i = 0;
for (auto& elem: rv) {
auto hi = ConstevalHexDigit(hex_str[i
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721460986)
I'm not advocating for it, but I think we can get it to compile by something like:
```C++
consteval auto operator"" _hex(const char* hex_str, size_t _)
{
constexpr std::size_t len = sizeof(hex_str);
static_assert(len % 2 == 0, "Hex string must have an even number of characters");
if (hex_str[len - 1] != '\0') throw "null terminator required";
std::array<std::byte, len / 2> rv{};
size_t i = 0;
for (auto& elem: rv) {
auto hi = ConstevalHexDigit(hex_str[i
...
🤔 ismaelsadeeq reviewed a pull request: "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`"
(https://github.com/bitcoin/bitcoin/pull/30667#pullrequestreview-2245023560)
Thank you for attempting to address this issue.
The changes in this PR are incorrect and do not address the issue.
Therefore, I am inclined to approach NACK.
If you have any questions about this PR, please ask them in the issue and review the guidelines at [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening a new PR.
Hint to anyone wanting to work on this.
check `MiniWallet::send_to` method.
https://github.com/bitcoin/bitcoin/blob/ee36717
...
(https://github.com/bitcoin/bitcoin/pull/30667#pullrequestreview-2245023560)
Thank you for attempting to address this issue.
The changes in this PR are incorrect and do not address the issue.
Therefore, I am inclined to approach NACK.
If you have any questions about this PR, please ask them in the issue and review the guidelines at [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening a new PR.
Hint to anyone wanting to work on this.
check `MiniWallet::send_to` method.
https://github.com/bitcoin/bitcoin/blob/ee36717
...
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721465852)
> more value in a test being consistent
Agree, that's why I'm suggesting this, [transaction_tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L913-L915) has many non-hexadecimal pushes.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721465852)
> more value in a test being consistent
Agree, that's why I'm suggesting this, [transaction_tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp#L913-L915) has many non-hexadecimal pushes.
💬 willcl-ark commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296076387)
Is the rationale proposed that it's more important for the node to be running than successfully to load a startup wallet? IMO loading a chosen wallet successfully would usually be more important.
If we changed to not fail startup the main effect would be that `bitcoind` would start up and sync further forwards, increasing the total work and time needed to resync _back_ to a consistent state for that wallet. Therefore it seems to make most sense that we fail "early and fast" if we need a resyn
...
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296076387)
Is the rationale proposed that it's more important for the node to be running than successfully to load a startup wallet? IMO loading a chosen wallet successfully would usually be more important.
If we changed to not fail startup the main effect would be that `bitcoind` would start up and sync further forwards, increasing the total work and time needed to resync _back_ to a consistent state for that wallet. Therefore it seems to make most sense that we fail "early and fast" if we need a resyn
...
💬 maflcko commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2296127510)
You re-ran the same task on the same commit on the same machine 3 hours later and it passed: https://cirrus-ci.com/task/6619444124844032?logs=ci#L313 vs https://cirrus-ci.com/task/5557228785106944?logs=ci#L311
Did you change anything in between?
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2296127510)
You re-ran the same task on the same commit on the same machine 3 hours later and it passed: https://cirrus-ci.com/task/6619444124844032?logs=ci#L313 vs https://cirrus-ci.com/task/5557228785106944?logs=ci#L311
Did you change anything in between?