💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676216295)
In https://github.com/bitcoin/bitcoin/blob/dddb59a63d90a108e7acefacdb6689a07324a6d7/src/coins.cpp#L193, we're calling `Next` with `erase`, which simply return `current.second.Next()` without any mutation, because of https://github.com/bitcoin/bitcoin/blob/dddb59a63d90a108e7acefacdb6689a07324a6d7/src/coins.h#L245, right?

This is the part I don't understand, especially since later in https://github.com/bitcoin/bitcoin/blob/dddb59a63d90a108e7acefacdb6689a07324a6d7/src/coins.cpp#L207, we're movi
...

📝 brunoerg opened a pull request: "test: addrman: tried 3 times and never a success so `isTerrible=true`"
(https://github.com/bitcoin/bitcoin/pull/30445)
This PR adds test coverage for the following verification:
```cpp
if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
return true;
}
```

If we've tried an address for 3 or more times and were unsuccessful, this address should be pointed out as "terrible".

-------

You can test this by applying:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 054a9bee32..93a9521b59 100644
--- a/src
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676224407)
Great question!

When `erase = true`, we will be erasing the entire map at the end of `BatchWrite`. When `erase = false`, we want to keep all unspent coins in the map and have their flags cleared.

While looping through `CCoinsMap::iterator`s, we can just do `it = erase ? mapCoins.erase(true) : std::next(it)`, and then deleting all spent coins and clear their flags right after we return from `BatchWrite` when `erase = false`.

Now that we are looping through `CoinsCachePair`s though, we ca
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2226020087)
Some benchmarks with the lastest changes. Did 2 IBD with `prune=550` and 2 `-reindex-chainstate` with `prune=0`. Results were similar to before. This time no prune was 1% faster on this branch with default `dbache`, and 1% slower on this branch with max `dbcache`. This just means there's no regression IMO.

It's 34% faster pruned with max `dbcache` and 13% faster with default `dbcache` on this branch vs a pruned node on master with max `dbcache`.

| | prune | dbcache | mean time (s) | speed
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676251092)
> , we're moving when erase = true because we've already erased it - right?

We're moving because we are going to erase it in either case, not that we have yet. We can't move an erased entry because it will have already been destroyed. And we can't use a moved entry later because it is invalidated by the move. So when we want to keep the entry, we need to copy it and not move out of it.
When `erase = true`, we will be erasing the entry after we return, so all entries can be moved. When `coin
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676259699)
I will have to read that again tomorrow, it's the end of the day here :/
`will_erase` could _help_, but I find it hard to keep track of so many independent mutations, I'm hoping there's a simpler way to do this...
Do you think you could add a test that at least doesn't pass for my suggested code above? Maybe that will help me understand why something this simple (iteration with/without simple/bulk cleanup) has this many loose parts, where the exact same parameter means the opposite from one li
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676292349)
You're right, the logic for moving or not should not leak into BatchWrite. Perhaps we can get rid of `erase` in BatchWrite and have the cursor return a pair - the iterator and a bool for whether it can move out or not?

💬 pinheadmz commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2226108900)
I am working on a libevent-replacing HTTP server using netbase primitives and yeah, I [have some functions](https://github.com/pinheadmz/bitcoin/blob/http-netbase/src/http.cpp#L311) that look a lot like the sv2connamn in this branch.

I think it would be cool if possible to abstract the mechanisms in `ThreadSocketHandler` to work on "abstract clients" and I'd be happy to review and collaborate on that.

💬 brunoerg commented on pull request "rest: Reject negative outpoint index early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2226117027)
Concept ACK

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676309999)
sounds better, would `Next` become a `const` in that case?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676321584)
Err, no `Next` would still conditionally erase entries. This would remove the `erase || it->second.coin.IsSpent()` condition in `BatchWrite`.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676324251)
Could we do that and delegate the deletion to a separate "service"? Seems complicated enough to warrant better encapsulation

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676328303)
I had it like that before but others suggested to move everything into 1 loop instead of looping through the dirty entries again after BatchWrite to clear flags and delete spent entries.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676330337)
Can we have it both, single loop, but the mutation and its side-effects (like the move) separated clearly in the code from everything else?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676334189)
Will give it some thought

💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2226183342)
Thanks @DrahtBot good bot, have a biscuit.

Rebased on master to fix silent conflict that failed function test. Also updated PR title and description since the name of the new field has changed

👍 pinheadmz approved a pull request: "Fix cases of calls to `FillPSBT` errantly returning `complete=true`"
(https://github.com/bitcoin/bitcoin/pull/30357#pullrequestreview-2175585156)
re-ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902

Changes since last ACK are minimal, slight simplification in tests to enable more legacy compatability

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaRgqoACgkQ5+KYS2KJ
yTq5NA/+NL1fOaCfl+P9yDIsXebxam/8Adf6iH5WUBt2TvUju06+305KL1dwvO6O
n0j8wmAeaJQe9tevuSTFYopxRaH+7
...

👍 pinheadmz approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2175592085)
ACK bca346a97056748f1e3fb899f336d56d9fd45a64

Changes since last ACK extremely minimal, cleaning up one line in the test

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK bca346a97056748f1e3fb899f336d56d9fd45a64
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaRhAIACgkQ5+KYS2KJ
yTr7nA//Sq7hxrI2I5/n/paZ+F6aMbZSxIkv7ynizzr3WfzBimoFmh+RcXkpyg5I
OK32DfJbKod4U1EgkTXY6RkjF8uYftCqLugF9qwDKC4NhkWQxEjepxX75Vrs
...

🤔 furszy reviewed a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2175592670)
q: what about disallowing the blocking-wait after stopping the scheduler? maybe only on debug mode. e.g. implementing an `isActive` method in the task runner and calling it prior to creating the promise.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1676377257)
Improved testing and error reporting in 01fa92928e6f9509935840a8c57d29774a0d14a1.

Made 3d0aec126f8103310b741a5c21e94cf537fcd191 use `util::Split`. It allocates a vector for each parsed UTXO but is more readable than my former approach.