💬 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.
(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.
(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
(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?
(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
...
(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
(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
(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.
(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
(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
(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?
(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
(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
(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);
```
(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.
(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.
(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.
(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
...
(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
...
💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398)
Nice catch! I think the changes in 744d7242239b8c09c2626e5ef7f90c2a91eed0fa might have some benefits, but might not be the best solution to this problem for a few reasons:
- They are not a complete fix. While they prevent an uncaught exception when you pass `-nowallet=not_a_bool` they do not prevent a similar uncaught exception when you pass `-nowallet=0`
- The new error message "Invalid '-no%s=%s': Value must be a boolean (0/1 or false/true)" seems to suggest that false/true values would be
...
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398)
Nice catch! I think the changes in 744d7242239b8c09c2626e5ef7f90c2a91eed0fa might have some benefits, but might not be the best solution to this problem for a few reasons:
- They are not a complete fix. While they prevent an uncaught exception when you pass `-nowallet=not_a_bool` they do not prevent a similar uncaught exception when you pass `-nowallet=0`
- The new error message "Invalid '-no%s=%s': Value must be a boolean (0/1 or false/true)" seems to suggest that false/true values would be
...
💬 ryanofsky commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212)
In commit "wallet: Write best block to disk before backup" (888a167446901c12960e6776613a3a1fb9789f59)
I wonder if it makes sense to put this in the CWallet::Flush method and call Flush() here. For example, this also seems like a good thing to do when unloading a wallet.
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212)
In commit "wallet: Write best block to disk before backup" (888a167446901c12960e6776613a3a1fb9789f59)
I wonder if it makes sense to put this in the CWallet::Flush method and call Flush() here. For example, this also seems like a good thing to do when unloading a wallet.