Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: replace crypto.HashObj(Transaction) with Transaction.ID() #3958

Merged
merged 3 commits into from
May 16, 2022
Merged

cleanup: replace crypto.HashObj(Transaction) with Transaction.ID() #3958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58c306e
replace crypto.HashObj(Transaction) with Transaction.ID()
brianolson May 6, 2022
0938248
fix
brianolson May 6, 2022
7af2a68
review tweaks: more test, less paren
brianolson May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/goal/clerk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ var groupCmd = &cobra.Command{
}

stxns = append(stxns, stxn)
group.TxGroupHashes = append(group.TxGroupHashes, crypto.HashObj(stxn.Txn))
group.TxGroupHashes = append(group.TxGroupHashes, crypto.Digest(stxn.ID()))
transactionIdx++
}

Expand Down
1 change: 1 addition & 0 deletions data/transactions/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, would be good to put that assertion in this PR

TxGroupHashes []crypto.Digest `codec:"txlist,allocbound=config.MaxTxGroupSize"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to make this []Txid?

}

Expand Down
18 changes: 18 additions & 0 deletions data/transactions/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,24 @@ func TestWellFormedErrors(t *testing.T) {
}
}

// TestTransactionHash checks that Transaction.ID() is equivalent to the old simpler crypto.HashObj() implementation.
func TestTransactionHash(t *testing.T) {
partitiontest.PartitionTest(t)

var txn Transaction
txn.Sender[1] = 3
txn.Fee.Raw = 1234
txid := txn.ID()
txid2 := Txid(crypto.HashObj(txn))
require.Equal(t, txid, txid2)

txn.LastValid = 4321
txid3 := txn.ID()
txid2 = Txid(crypto.HashObj(txn))
require.NotEqual(t, txid, txid3)
require.Equal(t, txid3, txid2)
}

var generateFlag = flag.Bool("generate", false, "")

// running test with -generate would generate the matrix used in the test ( without the "correct" errors )
Expand Down
15 changes: 8 additions & 7 deletions data/txntest/txn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,19 +269,20 @@ func SignedTxns(txns ...*Txn) []transactions.SignedTxn {
txgroup := transactions.TxGroup{
TxGroupHashes: make([]crypto.Digest, len(txns)),
}
stxns := make([]transactions.SignedTxn, len(txns))
for i, txn := range txns {
txn.Group = crypto.Digest{}
txgroup.TxGroupHashes[i] = crypto.HashObj(txn.Txn())
stxns[i] = txn.SignedTxn()
Copy link
Contributor

@cce cce May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why flip the order? (pulling txn.SignedTxn() up to happen before the ID stuff) I guess I don't understand this change

Copy link
Contributor

@jannotti jannotti May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
for i, txn := range stxns {
txn.Txn.Group = crypto.Digest{}
txgroup.TxGroupHashes[i] = crypto.Digest(txn.ID())
}
group := crypto.HashObj(txgroup)
for _, txn := range txns {
for i, txn := range txns {
txn.Group = group
stxns[i].Txn.Group = group
}

stxns := make([]transactions.SignedTxn, len(txns))
for i, txn := range txns {
stxns[i] = txn.SignedTxn()
}
return stxns

}
4 changes: 2 additions & 2 deletions ledger/internal/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,7 @@ func (eval *BlockEvaluator) TestTransactionGroup(txgroup []transactions.SignedTx
txWithoutGroup := txn.Txn
txWithoutGroup.Group = crypto.Digest{}

group.TxGroupHashes = append(group.TxGroupHashes, crypto.HashObj(txWithoutGroup))
group.TxGroupHashes = append(group.TxGroupHashes, crypto.Digest(txWithoutGroup.ID()))
} else if len(txgroup) > 1 {
return fmt.Errorf("transactionGroup: [%d] had zero Group but was submitted in a group of %d", gi, len(txgroup))
}
Expand Down Expand Up @@ -981,7 +981,7 @@ func (eval *BlockEvaluator) transactionGroup(txgroup []transactions.SignedTxnWit
txWithoutGroup := txad.SignedTxn.Txn
txWithoutGroup.Group = crypto.Digest{}

group.TxGroupHashes = append(group.TxGroupHashes, crypto.HashObj(txWithoutGroup))
group.TxGroupHashes = append(group.TxGroupHashes, crypto.Digest(txWithoutGroup.ID()))
} else if len(txgroup) > 1 {
return fmt.Errorf("transactionGroup: [%d] had zero Group but was submitted in a group of %d", gi, len(txgroup))
}
Expand Down
2 changes: 1 addition & 1 deletion libgoal/transactions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
}

return crypto.HashObj(group), nil
Expand Down