💬 achow101 commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3453170678)
ACK 8b892d41fdeb5756fd83f6050f27a170338d260a

💬 willcl-ark commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3453183249)
My guix build:

```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc5
...

💬 willcl-ark commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#discussion_r2467017701)
I guess this means our bins won't work on Ubuntu 22.04 LTS any more as they only have [2.35](https://repology.org/project/glibc/versions)?

LTS for that version persists until 2027 and we'd probably want to update before then anyway, so perhaps it's no issue.

💬 ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3453225085)
re: https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3384985314, https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3453042451

> ```diff
> + if (!interrupt_wait) throw std::runtime_error("Interrupt requested while no wait operation is active");
> ```

I think this would just re-introduce the race condition we were trying to get rid of. The waitNext call is asynchronous and there's no way for a client to know if a waitNext is active or not. They can only know if t
...

💬 l0rinc commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3453237274)
Rebased the change, all tests pass locally.
Reverted the first commit, the test did indeed fail as expected.
I have reviewed the test code lightly, seems roughly correct.

Nit: `assert_start_raises_init_error` originally mixed the terminology and claims the method "throws" and later that it "raises": if you touch again, consider unifying it to be [more pythonic](https://realpython.com/python-raise-exception/#raising-exceptions-in-python-the-raise-statement). Definitely not a blocker.

ACK
...

💬 l0rinc commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#discussion_r2466953638)
It's a bit weird that this mixin knows where it will be used, but I guess that's not gonna' change, so it's fine

🤔 mzumsande reviewed a pull request: "Rollback for dumptxoutset without invalidating blocks"
(https://github.com/bitcoin/bitcoin/pull/33477#pullrequestreview-3385298069)
Concept ACK

> I suspect if you go back further, this approach will end up performing better because we no longer need to roll back forward at the end

Would be interesting if someone could try out by going back 25k blocks or more, as is usually done for creating snapshots (I can't right now).

💬 mzumsande commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2466957433)
Don't we need to hold `cs_main` throughout? What if the utxo set changes while we are copying coins?

💬 mzumsande commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2466997313)
This seems unnecessary. As far as I can see the final `DisconnectBlock` call should have set the best block already, so that could we instead Assume here that `GetBestBlock == target->GetBlockHash()`.

🚀 achow101 merged a pull request: "wallet: allow label for non-ranged external descriptor (if `internal=false`) & disallow label for ranged descriptors"
(https://github.com/bitcoin/bitcoin/pull/31514)

💬 fanquake commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#discussion_r2467068897)
This PR doesn't change the runtime requirements of the binaries (see symbol-check. That comment is just a reference to the version of the Guix glibc package that is being inherited.

🤔 l0rinc reviewed a pull request: "validation: fetch block inputs on parallel threads >10% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3347436866)
Looks like I forgot to push the comments last time - that explains your surprise.

Since I'm reposting these old comments that I forgot last time (since I acked from the main view), lemme' post a recently finished Rpi4 benchmark showing a 31% speedup for an older push \:D/

<details>
<summary>31% faster reindex-chainstate | 919191 blocks | dbcache 450 | rpi4-8-1 | aarch64 | Cortex-A72 | 4 cores | 7.6Gi RAM | ext4 | SSD</summary>

```
Good news, the older version just finished on an rpi4
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442488159)
What's the point of the loop in these, is there a state we're changing in each iteration?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442436064)
I have played with this to see if we can construct the new cache layer before the fetcher, so that it populates the new layer instead of reading & writing to the old one - seemed like a cleaner separation.

But unfortunately we would need access to both in-memory cache layers inside `FetchInputs` in that case - to read for presence from the old cache and to write the newly fetched values to the new one (which will be flushed together with the new outputs when existing this scope).

But given tha
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2437808988)
As far as I understood the tx index and the vin index cannot exceed the limits of a `uint32_t`, see https://github.com/bitcoin/bitcoin/blob/e744fd1249bf9577274614eaf3997bf4bbb612ff/src/primitives/transaction.h#L32

Please consider `std::vector<std::pair<uint32_t, uint32_t>>` instead, which would likely halve the memory footprint. Based on https://godbolt.org/z/Wb918bWaM it seems to me this layout allows modern compilers to coalesce the two member accesses into a single, optimal 64-bit load fro
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442481463)
Does this mean the spent tx is never processed on the same thread currently?

Maybe we can mix it up a bit by something like
```C++
if (m_rng.randbool()) {
prevhash = tx.GetHash(); // TODO This can theoretically simulate double spends
}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442498069)
nit:

```suggestion
std::optional<Coin> GetCoin(const COutPoint&) const override { std::abort(); }
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442498626)
`GetCoin` shouldn't return spent entries

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442499107)
As mentioned before, I really dislike these lines, a fetcher shouldn't change the internal state (especially since they're const). Since we need a continuously running threads, can we package these to avoid state mutations? This would likely be solved by sending these off to a ThreadPool instance.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2442502361)
We shouldn't test this with invalid (empty) blocks:

```suggestion
if (block.vtx.empty()) continue;
fetcher.FetchInputs(cache, db, block);
```