✅ willcl-ark closed an issue: "doc: deduplicate list of chain/network strings in RPC/parameter help texts"
(https://github.com/bitcoin/bitcoin/issues/30645)
(https://github.com/bitcoin/bitcoin/issues/30645)
💬 willcl-ark commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.
💬 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
...