🤔 pablomartin4btc reviewed a pull request: "rpc: Do not wait for headers inside loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29345#pullrequestreview-1852045348)
Concept ACK

I agree with this change and @theStack's [comment](https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1917386986); one case I can think of would be a "ready to go" installation package or something like that? But as @maflcko said in the [description](https://github.com/bitcoin/bitcoin/pull/29345#issue-2105731808), the procedure/ RPC call would need to be re-executed again, so not too sure about it really.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917639308)
re: https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917606474

> Then we receive an out-of order block of height h (that doesn't connect yet) -> we set nTx and nChainTx.

I was going to reply that in this case we should only set `nTx` not `nChainTx`, but this does not appear to be true. The code that is trying to check for this case:

https://github.com/bitcoin/bitcoin/blob/cad2df24b396be403f13f372ec48ea14a90d7f06/src/validation.cpp#L3604-L3605

just assumes that if the pre
...

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1917663387)
> I don't think this is true. For example, it is possible that a block was downloaded and the `nTx` value was set, but the block later turned out to be invalid. Thus `IsValid()` will return false, but the `nTx` value is real.

Good point. Maybe the practical consequences would not be too bad if it just causes nTx to be omitted from rpc results when in some cases it shouldn't be. But this seems like another reason to want get rid of the fake assumeutxo nTx and nChainTx values, so we can simply
...

💬 maflcko commented on pull request "rpc: Do not wait for headers inside loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1917674791)
> Curious about reasonable arguments to keep it though

The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.

📝 maflcko converted_to_draft a pull request: "refactor: Remove nChainTx from consensus"
(https://github.com/bitcoin/bitcoin/pull/29349)
`nChainTx` is returned by RPCs and is used to estimate (sync and rescan) progress.

However, it is also used for consensus and P2P logic to denote that a block and all its parents have `BLOCK_VALID_TRANSACTIONS` set.

This is confusing, because:

* It would be clearer to use the `BlockStatus` enum for this as well.
* It requires AssumeUtxo to fake the `nChainTx` values.
* It makes it harder to remove `nChainTx` completely.

Fix it by introducing a new `BLOCK_VALID_TRANSACTIONS_TREE` le
...

💬 maflcko commented on pull request "refactor: Remove nChainTx from consensus":
(https://github.com/bitcoin/bitcoin/pull/29349#discussion_r1471817436)
Good catch. It should be possible to keep the persisted format by using a serialize helper that translates nStatus both ways losslessly and then recover `BLOCK_VALID_TRANSACTIONS_TREE` in `LoadBlockIndex` as a memory-only value.

Though, I wonder if this pull request is still worth it, if more code changes are needed.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471853965)
re: https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471199191

Thanks for clarifying.

> I wanted to emphasize the importance of rolling back the transaction by explaining the consequences of leaving the transaction dangling.

That seems ok. Would maybe suggest adding to the previously suggested comment:

* // If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case by closing and reopening the database. Closing t
...

👍 ryanofsky approved a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1852174380)
Code review ACK 756bed0529b46944162c02c783ecc804f802b637. New update consolidating the exec classes looks good.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471833134)
In commit "sqlite: add ability to interrupt statements" (177ff954c1221a00d4bf5ea63e6bfc415ba706c8)

It's technically not an abstract class anymore since it doesn't contain any `= 0` pure virtual methods. Would maybe suggest "Class responsible for executing SQL statements in SQLite databases. Methods are virtual so they can be overridden by unit tests testing unusual database conditions."

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471840452)
In commit "sqlite: add ability to interrupt statements" (177ff954c1221a00d4bf5ea63e6bfc415ba706c8)

This doesn't seem right because the `&&` operator will convert the integer return value into a bool. Also if m_exec_handler is null the `res` value will be 0 which is SQLITE_OK so the call will appear to succed.

It probably would make more sense to do `int res = Assert(m_exec_handler->Exec`

💬 Wwwwwwuuuuu commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1917783750)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125

💬 Wwwwwwuuuuu commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1917814292)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125

💬 Wwwwwwuuuuu commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1917817576)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1471908574)
double-checking: are we sure we can't hit this? @sdaftuar

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915202)
yes, fixed. Thanks. God knows why I did that.. it seems that I wasn't focused.

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915666)
yeah, fixed. I pasted your doc suggestion first, then made the classes consolidation..

📝 sr-gi opened a pull request: "test: fixes flaky wallet_import_rescan functional test"
(https://github.com/bitcoin/bitcoin/pull/29350)
wallet_import_rescan is, potentially, not rounding values used for `sendtoaddress` in one of the test cases. Under normal conditions, the value is randomly picked and rounded to 8 decimal positions, however, for this specific case the value is limited to a maximum value that is not properly rounded. If that value happens to be picked, the test will fail.

I noticed this in a completely unrelated PR where the test failed: https://cirrus-ci.com/task/5467779164274688?logs=ci#L2684

<!--
*** Pl
...

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471941289)
All good, applied your suggestion. Thanks!

> This is exactly what I think the comment should not be saying because the error cannot be transient. And it's confusing to suggest that it could be transient, because it either makes the rest of the code seem broken, or it makes this code seem unnecessary.

I was also thinking about an I/O hardware malfunctioning error, which could be transient; sqlite has return codes for it. But it's probably better to abort the program if something like this e
...

💬 Wwwwwwuuuuu commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917873911)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125

🤔 furszy reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1852350177)
Updated per feedback, thanks ryanofsky!
Docstrings were improved, and a bug related to a null `SQLiteExecHandler` has been fixed.
[Small diff](https://github.com/bitcoin/bitcoin/compare/756bed0529b46944162c02c783ecc804f802b637..b298242c8d495c36072415e1b95eaa7bf485a38a).