💬 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.

⚠️ 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
...

💬 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.

💬 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
...

🤔 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

👍 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
...

💬 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
...

💬 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
...

💬 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
...

🤔 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
...

💬 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.

💬 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
...

💬 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?

💬 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

💬 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
...

💬 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
...

💬 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
...

💬 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.

💬 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.

💬 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
...