💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2299620718)
> The error message could suggest an alternative path to follow, such as: "If you want to proceed without using this wallet, use '-disablewallet=<wallet_name>' during startup".

Why not automatically unloading a wallet if it prevents bitcoind from completing start-up?

💬 fjahr commented on issue "wallet: lastprocessedblock is be inconsistent with internal best block":
(https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299634338)
> Another more minimalistic approach could be to show users the relevant block locator. That may be in getwalletinfo() or the return value of backupwallet().

Here is rough draft of what that could look like. I am not if that is a good solution as mentioned above but if people think it's valuable I can clean it up and open a PR: https://github.com/fjahr/bitcoin/commit/fa4c9978a50802ad3ada5b6dce370f57b8ec16b8

💬 fjahr commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299635058)
> It would be great if we could open an issue describing the user behavior that's confusing.

I have created an issue here: #30686

💬 ryanofsky commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#issuecomment-2299641553)
Code review 9f19d10922d615c1e313eb7348fcc536642ad2e4.

I'm not sure this fix is ideal. For example if the GUI is used I believe this will change the error message from:

- Error: Invalid port specified in -rpcbind: '127.0.0.1:notaport'

to just:

- Unable to start HTTP server. See debug log for details.

I think it would be good to keep the new `return false` and error log in `HTTPBindAddresses`. But a more direct fix for this issue might be to just move the code that checks port numbe
...

💬 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);
```