💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917533420)
> Let me know if I should reopen this issue?

It's good to close. Extending the assert with https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 would only be a marginal improvement. I think a better long term fix would be to get rid of fake `nTx` and `nChainTx` values as described in https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108 "In the long run...".

💬 maflcko commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1917542993)
> It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.

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

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471690474)
Good catch, I think that is possible. Would just using wtxids everywhere here fix this?

💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917606474)
> // If block is assumed valid, nChainTx could be faked, but should at least be increasing between blocks.

I'm not completely convinced this was always true - I might well be wrong (didn't test it), but imagine the following scenario during the background sync:
In the beginning, all relevant blocks are assumed valid and have fake values for `nTx` and `nChainTx`.
Then we receive an out-of order block of height `h` (that doesn't connect yet) -> we set `nTx` and `nChainTx`.
-> Everything is
...

👍 stickies-v approved a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1852031524)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2

I would prefer the commit message to contain a rationale for the change, though. It's currently empty.

---

I now believe that the network-adjusted time implementation in master does not offer any security guarantees against an attacker trying to get a node out of consensus. The existing NTP vulnerabilities (which seem to be very real, even if currently apparently not exploited too often) can just as well be used by an attacker when network-ad
...

💬 sdaftuar commented on pull request "refactor: Remove nChainTx from consensus":
(https://github.com/bitcoin/bitcoin/pull/29349#discussion_r1471736433)
I'm trying to understand if it's safe to modify these values -- I believe we write the nStatus field to disk and load it on restart, so wouldn't changing these values require a block-index-upgrade mechanism to be created, in order to avoid problems on restart?

🤔 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