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.

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

transaction grouping support #172

Merged
merged 21 commits into from
Sep 11, 2019
Merged

transaction grouping support #172

merged 21 commits into from
Sep 11, 2019

Conversation

zeldovich
Copy link
Contributor

@zeldovich zeldovich commented Jul 22, 2019

No description provided.

@zeldovich zeldovich changed the title draft transaction grouping support transaction grouping support Aug 27, 2019
@zeldovich zeldovich marked this pull request as ready for review August 27, 2019 15:33
return pool.Remember([]transactions.SignedTxn{t})
}

// Remember stores the provided transaction group
// Precondition: Only Remember() properly-signed and well-formed transactions (i.e., ensure t.WellFormed())
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR's changes, but do you know if this precondition (check WellFormed before calling Remember) is still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is certainly misleading. The well-formedness doesn't matter much. It's critical that the transaction signature has been checked, though. The pool assumes the signature is already valid (doesn't do its own check), and the block evaluator subsequently assumes any transactions coming from the pool have good signatures.

@algoradam
Copy link
Contributor

To be sure I understand correctly, would the following be an accurate addition to the spec describing this change?

A transaction may include a "Group" field (msgpack tag "grp"), a 32-byte hash that specifies what transaction group the transaction belongs to.
Informally, a transaction group is an ordered list of transactions that, in order to be confirmed at all, must all be confirmed together, in order, in the same block. The "Group" field in each transaction is set to what is essentially the hash of the list of transaction hashes in the group, except to avoid circularity, when hashing a transaction it is hashed with its "Group" field omitted. In this way a user wanting to require transaction A to confirm if and only if transactions B and C confirm can take the hashes of transactions A, B, and C (without the "Group" field set), hash them together, and set the "Group" field of all three transactions to that hash before signing them.

More formally, when evaluating a block, consider the $i$th and $(i+1)$th transaction in the payset to belong to the same transaction group if the "Group" fields of the two transactions are equal and nonzero.
The block may now be viewed as an ordered sequence of transaction groups, where each transaction group is a contiguous sublist of the payset consisting of one or more transactions with equal "Group" field.
For each transaction group where the transactions have nonzero "Group", compute the TxGroup hash as follows:
1. Take the hash of each transaction in the group but with its "Group" field omitted.
2. Hash this ordered list of hashes -- more precisely, hash the canonical msgpack encoding of a struct with a field "txlist" containing the list of hashes, using "TG" as domain separation prefix.
If the TxGroup hash of any transaction group in a block does not match the "Group" field of the transactions in that group (and that "Group" field is nonzero), then the block is invalid.

@zeldovich
Copy link
Contributor Author

To be sure I understand correctly, would the following be an accurate addition to the spec describing this change?

Yes, that's accurate -- thank you for reverse-engineering that out of the code!

@algoradam
Copy link
Contributor

What's the intended flow for signing a group of transactions where e.g., one of the transactions must be signed by person A and another must be signed by person B? goal clerk sign will abort if you give it a file with multiple transactions and it can't sign them all; algokey will sign all the transactions with whichever key you give it (including the transactions for which that's the wrong key). There doesn't seem to be a goal command to split apart a file with multiple transactions in it.

@zeldovich
Copy link
Contributor Author

What's the intended flow for signing a group of transactions where e.g., one of the transactions must be signed by person A and another must be signed by person B? goal clerk sign will abort if you give it a file with multiple transactions and it can't sign them all; algokey will sign all the transactions with whichever key you give it (including the transactions for which that's the wrong key). There doesn't seem to be a goal command to split apart a file with multiple transactions in it.

Good point -- I'll add a goal command to split a transactions file into one file per txn.

(I was using msgpacktool to do this, but that's not something we should be suggesting to all of our users.)

unverifiedTxGroup := make([]transactions.SignedTxn, 1)
for {
if len(unverifiedTxGroup) == ntx {
n := make([]transactions.SignedTxn, len(unverifiedTxGroup)*2)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be sure I'm not missing something subtle here: Instead of decoding and calling append(), you're growing the array yourself so that you can decode directly into &unverifiedTxGroup[ntx], saving a memcpy, right? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's all that's going on here.

I believe the code was originally carefully optimized to the level where an extra copy of the SignedTxn mattered, so I tried to preserve that property.

@algoradam
Copy link
Contributor

It looks like there's currently no way to use goal to send a transaction group -- goal clerk rawsend tries to broadcast transactions individually.

@zeldovich
Copy link
Contributor Author

It looks like there's currently no way to use goal to send a transaction group -- goal clerk rawsend tries to broadcast transactions individually.

Good catch (thought I did this, but clearly not). I fixed rawsend to respect groups.

Copy link
Contributor

@algoradam algoradam left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@zeldovich zeldovich merged commit 28a4138 into algorand:master Sep 11, 2019
@zeldovich zeldovich deleted the txgroup branch September 11, 2019 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants