👍 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?
💬 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-2296135640)
Also, probably unrelated, but if you want, you can test https://github.com/bitcoin/bitcoin/pull/30639 and https://github.com/bitcoin/bitcoin/pull/30634
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2296135640)
Also, probably unrelated, but if you want, you can test https://github.com/bitcoin/bitcoin/pull/30639 and https://github.com/bitcoin/bitcoin/pull/30634
💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296139536)
> Is the rationale proposed that it's more important for the node to be running than successfully to load a startup wallet?
Yes. I would prefer bitcoind to run independently from a wallet by default. I don't see a reason for a wallet to drive behaviur of bitcoind. Also, blocked startup of bitcoind could be seen as a (minor?) security issue.
> Therefore it seems to make most sense that we fail "early and fast" if we need a resync (for any reason) that way minimising the work and bandwidth u
...
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296139536)
> Is the rationale proposed that it's more important for the node to be running than successfully to load a startup wallet?
Yes. I would prefer bitcoind to run independently from a wallet by default. I don't see a reason for a wallet to drive behaviur of bitcoind. Also, blocked startup of bitcoind could be seen as a (minor?) security issue.
> Therefore it seems to make most sense that we fail "early and fast" if we need a resync (for any reason) that way minimising the work and bandwidth u
...
💬 virtu commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2296145504)
ACK [51ad84a](https://github.com/bitcoin/bitcoin/commit/41ad84a00c20f54b520aab7f6f975231da0ee2d0)
Reviewed the code; changes look fine. Also in favor of using the regularly-updated asmap from collaborative runs.
I noticed the `seeds.txt.gz` file I used (~2024-08-19T08:30Z) file did not contain any good I2P nodes.
Also, since I've been working on on exporting my crawler's results as well, I noticed the Onion node numbers seem rather low. Here's some statistics that I generated by skippin
...
(https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2296145504)
ACK [51ad84a](https://github.com/bitcoin/bitcoin/commit/41ad84a00c20f54b520aab7f6f975231da0ee2d0)
Reviewed the code; changes look fine. Also in favor of using the regularly-updated asmap from collaborative runs.
I noticed the `seeds.txt.gz` file I used (~2024-08-19T08:30Z) file did not contain any good I2P nodes.
Also, since I've been working on on exporting my crawler's results as well, I noticed the Onion node numbers seem rather low. Here's some statistics that I generated by skippin
...
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296165565)
@marcofleon Another question I'd ask is: What software are you using? Apple-, Microsoft-, or "Anti-Virus"- software? They have a history of scanning, intercepting, and (temporarily) blocking file IO, so absent any more information or steps to reproduce, this would be my most likely guess. Most useful for me right now would be any kind of performance measurement on your machine, for example a flame graph of a fuzz run, or any other measurement that shows where the most time is spent.
(I may or
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296165565)
@marcofleon Another question I'd ask is: What software are you using? Apple-, Microsoft-, or "Anti-Virus"- software? They have a history of scanning, intercepting, and (temporarily) blocking file IO, so absent any more information or steps to reproduce, this would be my most likely guess. Most useful for me right now would be any kind of performance measurement on your machine, for example a flame graph of a fuzz run, or any other measurement that shows where the most time is spent.
(I may or
...
💬 maflcko commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296176138)
> Even if, then a wallet state could have been synced from chainstate/UTXO set with txindex without additional work and bandwith consumed.
Prune mode is incompatible with txindex, and will similarly abort the start. This means that if you had txindex enabled, prune mode would be disabled, and you wouldn't see the error in the issue description in the first place.
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296176138)
> Even if, then a wallet state could have been synced from chainstate/UTXO set with txindex without additional work and bandwith consumed.
Prune mode is incompatible with txindex, and will similarly abort the start. This means that if you had txindex enabled, prune mode would be disabled, and you wouldn't see the error in the issue description in the first place.
💬 Sjors 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-2296185391)
@maflcko yes, I first reproduced the issue and then tested the workaround `vm.mmap_rnd_bits=28`.
I'll try those clang-19 PRs.
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2296185391)
@maflcko yes, I first reproduced the issue and then tested the workaround `vm.mmap_rnd_bits=28`.
I'll try those clang-19 PRs.
💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296243876)
> > Even if, then a wallet state could have been synced from chainstate/UTXO set with txindex without additional work and bandwith consumed.
>
> Prune mode is incompatible with txindex, and will similarly abort the start. This means that if you had txindex enabled, prune mode would be disabled, and you wouldn't see the error in the issue description in the first place.
I agree (and to clarify the corner case: "wallet state could have been synced from chainstate/UTXO set with txindex" using
...
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296243876)
> > Even if, then a wallet state could have been synced from chainstate/UTXO set with txindex without additional work and bandwith consumed.
>
> Prune mode is incompatible with txindex, and will similarly abort the start. This means that if you had txindex enabled, prune mode would be disabled, and you wouldn't see the error in the issue description in the first place.
I agree (and to clarify the corner case: "wallet state could have been synced from chainstate/UTXO set with txindex" using
...
💬 maflcko commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296301079)
> The main point still holds: Bitcoind startup should not be terminated despite wallet synchronisation error.
I disagree that an error should be silently ignored during startup. If the user asked for something during startup, it seems clearer to tell them as soon as possible that the request could not be fulfilled. This allows them to evaluate their alternatives, or spot possible mistakes in their setup. If the user request is silently ignored, it may just lead to a different kind of error do
...
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296301079)
> The main point still holds: Bitcoind startup should not be terminated despite wallet synchronisation error.
I disagree that an error should be silently ignored during startup. If the user asked for something during startup, it seems clearer to tell them as soon as possible that the request could not be fulfilled. This allows them to evaluate their alternatives, or spot possible mistakes in their setup. If the user request is silently ignored, it may just lead to a different kind of error do
...
💬 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-2296308983)
I see. So in theory it should be reproducible by setting up a vanilla Ubuntu 24.04 (or later) *host* to run the CI tasks. I guess no one has done so yet, given that you are the first one to observe the issue. However, if it is reproducible, then it probably should be fixed.
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2296308983)
I see. So in theory it should be reproducible by setting up a vanilla Ubuntu 24.04 (or later) *host* to run the CI tasks. I guess no one has done so yet, given that you are the first one to observe the issue. However, if it is reproducible, then it probably should be fixed.
💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296327794)
> > The main point still holds: Bitcoind startup should not be terminated despite wallet synchronisation error.
>
> I disagree that an error should be silently ignored during startup.
I am also not advocating for silently ignoring wallet error during bitcoind startup. At the same time, I think that blocking bitcoind startup is not the same as alerting a user. Thankfully, operating system is not shut down due to Bitcoin Core Wallet error message.
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2296327794)
> > The main point still holds: Bitcoind startup should not be terminated despite wallet synchronisation error.
>
> I disagree that an error should be silently ignored during startup.
I am also not advocating for silently ignoring wallet error during bitcoind startup. At the same time, I think that blocking bitcoind startup is not the same as alerting a user. Thankfully, operating system is not shut down due to Bitcoin Core Wallet error message.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721650301)
I tried before your suggestion and then tried again when you suggested on all 3 compilers and none worked. Which I stated.
Then you pasted a longer example. Have you tested and got anything remotely close to that working on any compiler, C++ or other language? :)
Resolving.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721650301)
I tried before your suggestion and then tried again when you suggested on all 3 compilers and none worked. Which I stated.
Then you pasted a longer example. Have you tested and got anything remotely close to that working on any compiler, C++ or other language? :)
Resolving.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721654337)
I like that my version just forwards everything in one statement, while your leaves 3, duplicated. But either one works for me.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721654337)
I like that my version just forwards everything in one statement, while your leaves 3, duplicated. But either one works for me.