Reuse calculated tx id in executor

[MitchTurner]

#1112

[xgreenx]

Could you also update places like this?

[MitchTurner]

Yes. I had it originally and then removed it because I thought there was some ordering issues, but it was a simple fix in the end.

[MitchTurner]

Actually. I was right. The compute_inputs needs to happen before the into_checked_basic because the tx is being mutated.

So we can't get the tx id from the checked_tx, and if we were to add a tx_id calculation before compute_inputs it would get called twice every time. I recommend keeping it as is since it only get's called once, either when it errors or when it does the tx_check.

[xgreenx]

The into_checked_basic shouldn't modify tx_id, otherwise something is wrong in our logic=D

[MitchTurner]

Sorry. That's not what I'm saying.

compute_inputs modifies the tx. And that's the method that you are suggesting passing tx_id into. As you can see, the tests are failing if I do into_check_basic before compute_inputs.

If tx_id is calculated during into_check_basic and compute_inputs needs to happen before into_check_basic I don't recommend calculating tx_id before compute_inputs and only calling tx.id() if an error occurs.

That is how the current code works. So, I'm suggesting we don't make this change.

[xgreenx]

Modification from compute_inputs doesn't affect transaction id. You don't need to do into_check_basic before

[MitchTurner]

Right. But it always calculates and caches the tx_id during into_check_basic. So adding an additional tx.id() would be redundant.

In other words, can you explain what benefits we get from passing tx_id into compute_inputs instead of calculating it on the error? The only placecompute_inputs uses tx_id is in the error cases--which we can assume won't get called very often. I might be missing something.

[xgreenx]

After refactoring the TxPool, we don't need to call into_check_basic, so in the future, we will use a cached value each time=)

The idea is to minimize the number of .id(&self.config.transaction_parameters.chain_id) in the code. Because without this change, we already use cached value everywhere. We receive transactions from TxPool, and their id is calculated there.

[MitchTurner]

Okay. Cool, if original_tx has a cached id, then I take it back. I'm comfortable with that!