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

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