cleanup: replace crypto.HashObj(Transaction) with Transaction.ID() #3958
Conversation
brianolson
added
Team Carbon-11
tech debt
| @@ -161,6 +161,7 @@ type TxGroup struct { | |||
| // together, sequentially, in a block in order for the group to be | |||
| // valid. Each hash in the list is a hash of a transaction with | |||
| // the `Group` field omitted. | |||
| // These are all `Txid` which is equivalent to `crypto.Digest` | |||
There was a problem hiding this comment.
could you please add a test into transaction_test.go checking tx.ID() == crypto.HashObj(tx) ? In code I see they are the same but this is not enforced.
There was a problem hiding this comment.
agreed, would be good to put that assertion in this PR
brianolson
changed the title
| for i, txn := range txns { | ||
| txn.Group = crypto.Digest{} | ||
| txgroup.TxGroupHashes[i] = crypto.HashObj(txn.Txn()) | ||
| stxns[i] = txn.SignedTxn() |
There was a problem hiding this comment.
Why flip the order? (pulling txn.SignedTxn() up to happen before the ID stuff) I guess I don't understand this change
There was a problem hiding this comment.
I get the impression that the intent was to get things ready so that once the group was calculated it could be put into the txn objects and the stxn objects, saving a loop. I think that would work. But this change doesn't save a loop, because of the loop used to initialize stxns. I think that loop could be removed, and the initialization done in the final loop (at what would now be line 282.5). Then you would indeed have the benefit of having eliminated a loop.
(Though I should point out this is entirely test code - note sure it needs to be faster.)
There was a problem hiding this comment.
Fewer copies. The local Txn object does a bunch of work inside .Txn() or .SignedTxn() to construct the data/transactions object. This ordering eliminates a call to .Txn() to build an object, and just uses thee data/transactions.SignedTxn to get the txid from.
libgoal/transactions.go
Outdated
| @@ -752,7 +752,7 @@ func (c *Client) GroupID(txgroup []transactions.Transaction) (gid crypto.Digest, | |||
| return | |||
| } | |||
|
|
|||
| group.TxGroupHashes = append(group.TxGroupHashes, crypto.HashObj(tx)) | |||
| group.TxGroupHashes = append(group.TxGroupHashes, crypto.Digest((tx.ID()))) | |||
There was a problem hiding this comment.
Is there an extra set of parens here?
| @@ -161,6 +161,7 @@ type TxGroup struct { | |||
| // together, sequentially, in a block in order for the group to be | |||
| // valid. Each hash in the list is a hash of a transaction with | |||
| // the `Group` field omitted. | |||
| // These are all `Txid` which is equivalent to `crypto.Digest` | |||
| TxGroupHashes []crypto.Digest `codec:"txlist,allocbound=config.MaxTxGroupSize"` | |||
There was a problem hiding this comment.
would it make sense to make this []Txid?
Codecov Report
@@ Coverage Diff @@
## master #3958 +/- ##
==========================================
+ Coverage 49.81% 49.82% +0.01%
==========================================
Files 409 409
Lines 68929 68929
==========================================
+ Hits 34335 34347 +12
+ Misses 30882 30879 -3
+ Partials 3712 3703 -9
Continue to review full report at Codecov.
|
Summary
For clarity, always use Transaction.ID() when generating the txid hash of a Transaction.
crypto.HashObj(txn)is equivalent but less commonly used and can lead one on a wild goose chase to determine, "wait, is that equivalent?"Test Plan
No functional change should be in this change. No new features. Should just pass existing tests.