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

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

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

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

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

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

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721677058)
In my suggestion there is not `std::span` involved and the size + data writing parts are split.

👍 danielabrozzoni approved a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2245365008)
tACK 07365d026127eb8435e7919a7793bd25d7f3ceca

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721678712)
The above seemed to be working (that's why I suggested it originally) - but I'm not in love with this version either - thanks for considering.

💬 Sjors commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2296430206)
Concept ACK.

In my testing of #30664 I keep seeing `share/rpcauth/__pycache__/` in `git status`, so that might need to be added to `.gitignore`.

🤔 paplorinc reviewed a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-2245093564)
The code became a lot simpler now - have a few suggestion I'd like us to consider, but I understand if you think it's out of scope.

Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.

💬 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_r1721733726)
```suggestion
Assume(IsDirty());
```

nit: it's a cheap call, why not make it an assert instead?

💬 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_r1721727681)
I was surprise to see that the method called `GetCoin` now:
* overwrites the dummy coin parameter with the value found in the cache
* returns a boolean indicating whether the coin exists in the cache and is not spent (the name doesn't have any hints about it being spent or not)

I don't mind if you think it's outside the scope of the current change, but checking the usages it seems to me we can now simplify `GetCoin` to actually return the coin and let the callsite check for spent and it mig
...

💬 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_r1721668812)
So basically if we lay out the allowed combinations we'd get:
```C++
assert((!entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 0
(entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 1
(entry.IsDirty() & entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 3
(entry.IsDirty() & !entry.IsFresh() & entry.coin.IsSpent())); // attr = 5
```
Which could be simplified to
```C++
assert((entry.IsDirty() && !(entry.coin.IsSpen
...