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

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1676379848)
e9a436b7796cb285730d179fb3317ad50234ab04 changed it to: `This is not a uint256 constructor because of historical fears of uint256(0) resolving to a NULL string and crashing.`

💬 pinheadmz commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2226264572)
concept ACK, this simplifies a confusing behavior with `rpcuser` misconfiguration. Code changes look simple enough, will test locally and run through your (very exhaustive!) behavior comparison

💬 mzumsande commented on pull request "init: change shutdown order of load block thread and scheduler":
(https://github.com/bitcoin/bitcoin/pull/30435#issuecomment-2226324701)
> It would help us catch these type of errors (if we still have them).

Do you mean throwing an assert instead of blocking indefinitely? That might be more convenient than hanging indefinitely, but I'm not sure if this really makes much of a difference in practice, because both ways should be easily recognizable both by users and tests.

An alternative approach would be to _allow_ it - just return instead of waiting forever, if we are in Shutdown mode and rely on a later `FlushBackgroundCal
...

👍 hebasto approved a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2175766383)
re-ACK 5fd48360198d2ac49e43b24cc1469557b03567b8.