💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659835029)
It just checks that removing the last element sets head to `nullptr`. I suppose just for completeness, since this case is the same as when we delete `n4` above.

👍 tdb3 approved a pull request: "test: p2p: check that connecting to ourself leads to disconnect"
(https://github.com/bitcoin/bitcoin/pull/30362#pullrequestreview-2149639061)
ACK 5d2fb14bafe4e80c0a482d99e5ebde07c477f000
Good addition.
Tested locally with the following:
1) Running the p2p_handshake test variants with `test_runner` (passed)
2) Running a purposefully separate node (`src/bitcoind -regtest -bind=127.0.0.1:1234`), and modifying `p2p_handshake` to run the test against it (`node_listen_addr = f"127.0.0.1:{1234}"`). Test failed as expected (expected messages weren't found).
3) Stopping the node at 1234 and running the modified test. Test failed as expe
...

💬 tdb3 commented on pull request "test: p2p: check that connecting to ourself leads to disconnect":
(https://github.com/bitcoin/bitcoin/pull/30362#discussion_r1659838824)
I like how this is using `self.options.v2transport` as the argument, enabling the script list in `test_runner` to specify whether or not v2transport should be used for the test.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659847033)
This checks that the linked list nodes get removed from the linked list due to the list iterator deleting them, not due to clearing the flags in `Next`.

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

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

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

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659853149)
Updated the comment.

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

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

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

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

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

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

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659854749)
Not sure I want to touch this right now. C

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

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659878816)
Thank you, let's see if others want this change too. I think it could be acceptable to have more example lines.

💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2198239776)
Force-pushed to add some `Assume`s. Looks like the CI issue was unrelated.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659918531)
Before this patch, the iteration is `it = erase ? mapCoins.erase(it) : std::next(it);`. So this is basically the same behavior, is it not?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659919946)
We want to tie adding a flag to adding the entry to the linked list. We don't want to have an interface that lets something add a flag without adding it to the linked list. I think splitting that functionality into multiple methods would be dangerous.

`ClearFlags` is similar. Does it matter to external callers how these methods modify the linked list? That should be transparent to callers. They just know the calling `Next()` from the head gets all flagged entries. The how should not be expose
...