Replace TxAccountUpdate and BlockAccountUpdate with AccountUpdate

[PhilippGackstatter]

Replace TxAccountUpdate and BlockAccountUpdate with AccountUpdate.

With the introduction of the batch kernel we would possibly need a third BatchAccountUpdate type which would look similar to the existing account update types, so it seemed useful to refactor the existing two into a single type that will also be usable for the batch kernel.

This uses the terminology of commitment over hash, i.e. final_state_commitment rather than final_state_hash. This is in anticipation of #1092 to avoid extra work when implementing that PR.

Part of #1095.

[bobbinth]

Looks good! Thank you! No specific comments - but I did want to bring up one point:

The main reason why TxAccountUpdate and BlockAccountUpdate were different is that for BlockAccountUpdate we didn't track the initial state of the account because it was retrieved from the database during block building. So, technically now the block includes some data that is not really needed.

Maybe that's fine, but wanted to bring it up to make sure we make a conscious decision on this. Or maybe there is a way to make it work somehow for both use cases.

[PhilippGackstatter]

The main reason why TxAccountUpdate and BlockAccountUpdate were different is that for BlockAccountUpdate we didn't track the initial state of the account because it was retrieved from the database during block building. So, technically now the block includes some data that is not really needed.

Good point. I glossed over this a bit while implementing this.
For a large block with a large number of updated accounts this would indeed add unnecessary data, in the absolute worst case 2 MiB (MAX_ACCOUNTS_PER_BLOCK * 32).

One way to fix this would be to remove the initial state commitment from the AccountUpdate type and make a wrapper for use in transaction and batches:

pub struct TraceableAccountUpdate {
    /// Commitment to the state of the account before this update is applied.
    ///
    /// Equal to `Digest::default()` for new accounts.
    initial_state_commitment: Digest,

    update: AccountUpdate,
}

One small issue is that the list of transactions that contributed to an account update is also unnecessary for the *Transaction types, although here it could be set to Vec::new(). That seems like a confusing API though, as one would expect it to return something. Especially when merging multiple account updates from different transactions into one, a caller would have to be aware they'd have to supply the transaction IDs themselves which is a subtle thing.

So there are some minor things that make me now think the cleanest solution is to just keep the individual types and have one for tx, batch and block account updates.

[bobbinth]

So there are some minor things that make me now think the cleanest solution is to just keep the individual types and have one for tx, batch and block account updates.

I'm fine with this. We may come back to a unified AccountUpdate struct if we go with the VerifiedTransaction approach, but that would be a separate PR.

[PhilippGackstatter]

We may come back to a unified AccountUpdate struct if we go with the VerifiedTransaction approach, but that would be a separate PR.

Agreed. Closing the PR.