protocol/bc: add SpendCommitment type #665

erykwalder · 2017-03-02T18:06:26Z

Spend TxInputs need to contain all of the data necessary to reconstruct
output ids in order to spend them. Performing the hashing during
validation gives us cryptographic integrity that the spent output data
is correct.

In order to preserve this data, the mapping process was updated to
return additional information which is stored in the account_utxos
table. This data is then populated in a SpendCommitment type on the
TxInput structs.

Fixes #648

bobg · 2017-03-02T18:09:19Z

👀

bobg

This is really nice work, thanks for doing it. I only have a few small comments, and one request: will you cherrypick (and adapt) 210dc19 to help use feel sure we're fixing what we expect to be fixing?

Oh, also mention in the PR description that this fixes #648

bobg · 2017-03-02T18:31:28Z

protocol/bc/txhashes.go

 			ExpirationMS uint64
 		}
-		VMContexts []*VMContext // one per old-style Input
+		SpentOutputIDs []Hash


Do the entries in this list correspond 1:1 with the inputs of the oldtx? (And for inputs that aren't spends, are those entries the zero hash?) Please add a comment saying so.

bobg · 2017-03-02T18:33:34Z

protocol/bc/txhashes.go

 // to avoid a circular dependency between the bc and tx packages.
 var BlockHeaderHashFunc func(*BlockHeader) Hash
+
+var OutputHash func(*SpendCommitment) (Hash, error)


Please godoc this var.

bobg · 2017-03-02T18:34:22Z

protocol/bc/spend.go

+	})
+}
+
+func (sc *SpendCommitment) Hash(suffix []byte, assetVersion uint64) (spendhash Hash) {


Am I correct in thinking that we don't expect this to get used? That it's only for the !SerPrevout case?

bobg · 2017-03-02T18:37:00Z

protocol/tx/map.go

 	for i, inp := range tx.Inputs {
 		if oldSp, ok := inp.TypedInput.(*bc.SpendInput); ok {
+			var oldSpID bc.Hash
+			oldSpID, err = ComputeOutputID(&oldSp.SpendCommitment)


In the absence of distinguishing types for outputIDs and IDs of other kinds of entry, please observe a convention of putting "out" somewhere in the varname when it holds an outputID. As it is, oldSpID looks like it could be the ID of a spend entry.

bobg · 2017-03-02T18:38:14Z

protocol/tx/transaction.go

+	}, program{VMVersion: sc.VMVersion, Code: sc.ControlProgram}, sc.RefDataHash, 0)
+
+	h = entryID(o)
+	return h, nil


Specifying nil here will override the setting of err that happens in the defer above. A bare return should do the trick.

I think the defer actually takes precedence:
https://play.golang.org/p/yHPkJUHgkn

The defer executes after the return statement, so that you can modify the returned values even when they've been supplied directly to return.

Oh of course - if recover() returns a value then we're in a panic and normal control flow from the entryID call to the return call didn't happen. Duh, sorry.

That's also true, but note that defer runs after return even when we're not in a panic. Along the lines of @erykwalder's example: https://play.golang.org/p/_qBWbRSA2x

bobg · 2017-03-02T19:08:56Z

protocol/tx/map.go

 		if oldSp, ok := inp.TypedInput.(*bc.SpendInput); ok {
-			var oldSpID bc.Hash
-			oldSpID, err = ComputeOutputID(&oldSp.SpendCommitment)
+			var spendOutputID bc.Hash


nit: spentOutputID would be better

bobg · 2017-03-02T19:11:15Z

LGTM (with one tiny nit) but please wait for other 👀 to take a look too

kr · 2017-03-02T19:16:35Z

👀

jbowens · 2017-03-02T19:26:58Z

core/migrate/data.go

+		ALTER TABLE account_utxos
+			ADD COLUMN source_id bytea NOT NULL,
+			ADD COLUMN source_pos bigint NOT NULL,
+			ADD COLUMN ref_data_hash bytea NOT NULL;


We expect customers to completely drop the database and recreate it before attempting to run a 1.1.1 binary right? I think that's a fine assumption when dealing with a blockchain reset.

Once landed, we should double check that the migration has the same name and hash on the 1.1.1 branch and main so that Cores updating from 1.1.1 to 1.2.0 do not try to run this migration again.

kr · 2017-03-02T19:51:49Z

It looks like the base branch for this PR is main. It's more important right now to be preparing a fix for the 1.1.x branch. Check out docs/internal/package-tags-branches.md and #670. Maybe also wait until the fallout from #670 is cleaned up before landing this? (And we might have to open a new PR anyway. Is there a way to change the base branch of an open PR?)

bobg · 2017-03-02T19:53:32Z

Is there a way to change the base branch of an open PR?

Yep, there's a base-branch pull-down menu when you click the Edit button next to the PR title.

Spend TxInputs need to contain all of the data necessary to reconstruct output ids in order to spend them. Performing the hashing during validation gives us cryptographic integrity that the spent output data is correct. In order to preserve this data, the mapping process was updated to return additional information which is stored in the account_utxos table. This data is then populated in a SpendCommitment type on the TxInput structs. 1.1-stable version of #665 Closes #677

Spend TxInputs need to contain all of the data necessary to reconstruct output ids in order to spend them. Performing the hashing during validation gives us cryptographic integrity that the spent output data is correct. In order to preserve this data, the mapping process was updated to return additional information which is stored in the account_utxos table. This data is then populated in a SpendCommitment type on the TxInput structs.

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable).

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). Closes #703

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). This is a 1.1-stable backport of #703.

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). This is a 1.1-stable backport of #703. Closes #704

erykwalder force-pushed the spent-output branch from 8444a22 to 710f595 Compare March 2, 2017 18:13

erykwalder added the PTAL label Mar 2, 2017

bobg reviewed Mar 2, 2017

View reviewed changes

bobg approved these changes Mar 2, 2017

View reviewed changes

jbowens reviewed Mar 2, 2017

View reviewed changes

jbowens approved these changes Mar 2, 2017

View reviewed changes

erykwalder changed the base branch from main to 1.1-stable March 2, 2017 21:37

erykwalder changed the base branch from 1.1-stable to main March 2, 2017 21:41

erykwalder mentioned this pull request Mar 2, 2017

protocol/bc: add SpendCommitment type #677

Merged

bobg added PTAL and removed PTAL labels Mar 3, 2017

erykwalder added 4 commits March 3, 2017 17:49

fix accidental delete

077fadb

doc it up

f190d29

s/d/t throughout the codebase. jk

a710468

iampogo force-pushed the spent-output branch from f507184 to a710468 Compare March 3, 2017 17:49

iampogo merged commit 28f6f9b into main Mar 3, 2017

iampogo deleted the spent-output branch March 3, 2017 17:51

jeffomatic added a commit that referenced this pull request Mar 6, 2017

core: tick network RPC version to 3

779ab59

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable).

jeffomatic mentioned this pull request Mar 6, 2017

core: tick network RPC version to 3 #703

Merged

iampogo pushed a commit that referenced this pull request Mar 6, 2017

core: tick network RPC version to 3

6ccaf4a

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable).

iampogo pushed a commit that referenced this pull request Mar 6, 2017

core: tick network RPC version to 3

325d8d6

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). Closes #703

jeffomatic added a commit that referenced this pull request Mar 6, 2017

core: tick network RPC version to 3

bed851a

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). This is a 1.1-stable backport of #703.

jeffomatic mentioned this pull request Mar 6, 2017

core: tick network RPC version to 3 #704

Merged

iampogo pushed a commit that referenced this pull request Mar 6, 2017

core: tick network RPC version to 3

4402f0f

This accounts for updates affecting validation-critical hashing in #665 (#677 in 1.1-stable). This is a 1.1-stable backport of #703. Closes #704

protocol/bc: add SpendCommitment type #665

protocol/bc: add SpendCommitment type #665

Uh oh!

Conversation

erykwalder commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobg commented Mar 2, 2017

Uh oh!

bobg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobg commented Mar 2, 2017

Uh oh!

kr commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kr commented Mar 2, 2017

Uh oh!

bobg commented Mar 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erykwalder commented Mar 2, 2017 •

edited

Loading