height_consistency constraint blocking sync when code updates two columns independently

[AArnott]

When a transaction is to be updated for the block it is actually mined in, this code executes:

librustzcash/zcash_client_sqlite/src/wallet.rs

Lines 2313 to 2338 in 8b49ca8

	TransactionStatus::Mined(height) => {
	// The transaction has been mined, so we can set its mined height, associate it with
	// the appropriate block, and remove it from the retrieval queue.
	let sql_args = named_params![
	":txid": txid.as_ref(),
	":height": u32::from(height)
	];
	conn.execute(
	"UPDATE transactions
	SET mined_height = :height
	WHERE txid = :txid",
	sql_args,
	)?;
	conn.execute(
	"UPDATE transactions
	SET block = blocks.height
	FROM blocks
	WHERE txid = :txid
	AND blocks.height = :height",
	sql_args,
	)?;
	notify_tx_retrieved(conn, txid)?;
	}

Notice how two distinct SQL statements are executed, one per column to be updated.
But this creates a constraint violations in the transactions table, which requires that block and mined_height be in sync:

CONSTRAINT height_consistency CHECK (block IS NULL OR mined_height = block)

In the repro, a user's wallet in unable to sync beyond a particular block number because this violation fires and breaks storing any more transactions in the db.

The new block # being set is just +1 of what is already there. Both block and mined_height columns are set to n in the transaction table, and in this code, the height parameter that is being applied to the db is n + 1.

Maybe there is a bug here in that both columns were set to n in the first place, where if the value wasn't final, perhaps mined_height shouldn't have been set. Or maybe a re-org changed the mined height such that it needed to be set again.
It seems to me that this code should be more resilient, by updating both columns atomically so as to not upset the table constraint.

[AArnott]

And incidentally, I can't see what value bringing in the blocks table adds to the UPDATE query here:

librustzcash/zcash_client_sqlite/src/wallet.rs

Lines 2329 to 2333 in 8b49ca8

	"UPDATE transactions
	SET block = blocks.height
	FROM blocks
	WHERE txid = :txid
	AND blocks.height = :height",

Why not simplify it to just this:

                 "UPDATE transactions 
                  SET block = :height
                  WHERE txid = :txid", 

Which then becomes trivial to join with the other query for one atomic operation:

                 "UPDATE transactions 
                  SET mined_height = :height, block = :height
                  WHERE txid = :txid", 

[AArnott]

I also note that the blocks table has a row with height equal to n, but it has no row for n + 1.
This causes an experimental change I applied to make the change atomically fail for yet another reason: an FK violation due to a missing blocks.height match.

[nuttycom]

The invariant of the blocks table is that an entry can only exist for a block that the wallet has scanned. The join to the blocks table is to ensure that the foreign key relationship between the transaction and the block is correctly established, and the check constraint then ensures that the relationship between the mined_height and block column is maintained.

The wallet may know about a transaction and the block that it was mined in without having scanned that block.

The problem here isn't with the database constraints; it's a bug somewhere in the assumptions about the database state at the time that this update is being performed. The constraints are guarding against database corruption, and thereby revealing this bug, but they're not the source of the problem.