💬 ryanofsky commented on issue "wallet: lastprocessedblock is be inconsistent with internal best block":
(https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299674756)
Thanks for explaining all this.

fa4c9978a50802ad3ada5b6dce370f57b8ec16b8 could solve the immediate problem if users notice the return value, but it doesn't seem as robust a solution as updating the wallet before creating the backup like 4ffdc9b70711b2bec0e6e86a4f2cc0a87de406d2 is trying to do. Only problem with that commit is it might not be writing the right locator:

```c++
if (m_chain) {
WalletBatch batch{GetDatabase()};
CBlockLocator loc{m_chain->getTipLocator()};
...

👍 brunoerg approved a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2249000981)
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

💬 furszy commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2299678390)
> Why not automatically unloading a wallet if it prevents bitcoind from completing start-up?

Wallets contain users' money. We should not silently unload any of them without user interaction. Their importance is greater than completing a node startup. Imagine how would you behave if you start your wallet's node and your coins are not there.

💬 ryanofsky commented on issue "wallet: lastprocessedblock is be inconsistent with internal best block":
(https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299685579)
I just noticed #30678. It may be good to reopen that.

💬 ryanofsky commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1723927691)
re: https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1722605697

> Should lock the wallet mutex and obtain the wallet's best locator from the wallet's last processed block (`m_last_block_processed`).

Left a similar suggestion https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299674756 implementing that

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723403236)
> Here, we want to avoid direct parent/child relationships between transacti

doesn't a chain mean direct parent/child?

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2248150197)
ok I reviewed the unit and fuzz tests.

For the tx_download fuzz target there's a couple issues:

1) the time could adversarially be moved forward only 1ms at a time, 100k iterations results in 100 seconds. I hacked the target to *only* allow the forced good peer to respond to the getdata. I got a case where the while loop exits when only 100 seconds have passed in simulated time. Stalling peer A refuses to send TX, then after 60 seconds, at the same time, an honest peer and stalling peer co
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723414301)
ptx/txid/wtxid can just be made once outside the loop since it's the same each time

as well as nodeid, now

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723401360)
simplest is a blank CScript

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723435539)
what is this chaining doing? I only see the ultimate tx being used anyways.

It might be useful to have the parent tx rejected optionally, to check that `TX_MISSING_INPUTS,` fills out txid and wtxid reject filters properly.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723456852)
1 2, 3 4s, 5 6s
womp womp

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723471572)
m_senders.back() should be <= NUM_PEERS

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723465940)
500 outputs is probably too much unless you think it's gaining some meaningful coverage?

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723496334)
`MempoolRejectedTx` should probably `Assume` that state is invalid at the top of its definition

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723473284)
not just ahead due to negative skips

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1723967099)
nit:
```suggestion
SetMockTime(base.nTime);
```

👍 brunoerg approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2249099464)
ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff

Left it running for a while, got a clear improvement compared to master.

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299770686)
> +0.04% seems okay if you ask me.

Maybe, but I was expecting smaller, that's why I started digging.

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2299849221)
Reopening due to interest from @ryanofsky in https://github.com/bitcoin/bitcoin/issues/30686. I implemented the suggestion improvements and expanded the test a little including some more detailed comments. If this is an improvement already maybe this has a chance to improve things while the approach of #30221 is still being discussed.

📝 fjahr reopened a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678)
I discovered that we don't write the best block to disk when trying to explain the behavior described here: https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882

In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in `wallet_assumeutxo.py` doesn't actually test what it says. It only fails because the best block isn't written and actually, the height of the backup that is loaded is at the snapshot height during
...