protocol/bc: reorganize and refactor #349

bobg · 2016-12-20T03:44:54Z

Reorganize and refactor TxData, TxInput, TxOutput, and related types in anticipation of future changes. Fix serialization errors and update test vectors.

Closes #330.
Closes #335.

bobg · 2016-12-20T04:00:23Z

PTAL

bobg · 2017-01-10T19:21:25Z

PTAL

bobg · 2017-01-10T21:50:09Z

rebased, PTAL

kr · 2017-01-10T22:55:10Z

It would be a lot easier to review this if the refactoring could be done separately from any behavior changes.

Unrelated thought, if you're looking for opportunities to pick off smaller pieces: how about a PR that just introduces the Read- and WriteVarstrList functions and uses them were applicable?

kr · 2017-01-10T22:50:03Z

protocol/bc/asset_witness.go

+	"chain/errors"
+)
+
+type AssetWitness struct {


For the most part, this PR seems to be moving our Go types to more closely match the datatypes described in the spec, but AssetWitness is an exception. What is it?

It would be good to have godoc, at least for newly-introduced types. The ones that do map 1-1 can be short, like "Type Foo represents a Foo as defined in Chain Protocol. See https://chain.com/docs/protocol/specifications/data."

IssuanceWitness got turned into AssetWitness somewhere between brain and keyboard, and stuck unnoticed somehow. Now fixed. Also added some Godoc.

bobg · 2017-01-11T19:39:43Z

!PTAL
See #376 first

bobg · 2017-01-13T20:51:54Z

Rebased, PTAL

bobg · 2017-01-17T18:38:12Z

PTAL

tessr · 2017-01-17T19:40:29Z

👮‍♀️ Hello, it's me, the PR size police!

Things that could get pulled out of this PR and into their own PRs:

Improving error messages (https://github.com/chain/chain/pull/349/files#diff-60836afb202f0dcd9b8af17f1788a860R86, https://github.com/chain/chain/pull/349/files#diff-016bd9f722295a39e81867b58707e4c4L117)
Moving sighasher out of transaction.go and into its own file (https://github.com/chain/chain/pull/349/files#diff-016bd9f722295a39e81867b58707e4c4L260)
Moving AssetAmount out of transaction.go and into asset.go (https://github.com/chain/chain/pull/349/files#diff-016bd9f722295a39e81867b58707e4c4L361)
Moving OutputCommitment out of TxOutput and into its own file (https://github.com/chain/chain/pull/349/files#diff-9ae5ef8a42069d22d4a6863875362c74L22), and the removal of (TxOutput).WriteCommitment (https://github.com/chain/chain/pull/349/files#diff-9ae5ef8a42069d22d4a6863875362c74L104)
(TxInput).WitnessHash and subsequently CalcMerkleRoot returning an error (https://github.com/chain/chain/pull/349/files#diff-601ce85b9d920fdbec9f8d7d0fd5950bR301 and https://github.com/chain/chain/pull/349/files#diff-9dec076f454f1bf45901947d56f2612aR17, respectively)
SpendInput and IssuanceInput getting moved into their own files (https://github.com/chain/chain/pull/349/files#diff-601ce85b9d920fdbec9f8d7d0fd5950bL23).

I think once some of these changes go in on their own, it will be easier to review any other smaller and nitty-grittier changes. I can open PRs for any of them.

Part of #349. Closes #393

From #349. Closes #394

Part of #349 Closes #395

Part of #349. Note that this simply splits out `OutputCommitment` without making some of the other changes to the output commitment that are included in 349. Closes #397

This trickles down to `CalcMerkleRoot` as well. Part of #349. Closes #398

Part of #349. In 349, there are other changes to both `IssuanceInput` and `SpendInput`, but I don't make those changes here. Closes #399

…e TxOutput witness??

bobg · 2017-01-19T21:04:15Z

Rebased: +585,-428

PTAL

tessr · 2017-01-19T21:52:28Z

protocol/bc/hash.go

@@ -96,7 +96,7 @@ func ParseHash(s string) (h Hash, err error) {
 	return h, errors.Wrap(err, "decode hex")
 }

-func writeFastHash(w io.Writer, d []byte) error {
+func WriteFastHash(w io.Writer, d []byte) error {


Does this need to be exported?

Nope, fixed, thanks.

tessr · 2017-01-23T18:33:42Z

protocol/bc/txinput.go

 	}
 	return o
 }
+
+func (t *TxInput) InitialBlock() (blockID Hash, ok bool) {


Is this used?

Nope, removed, thanks.

tessr · 2017-01-23T18:33:54Z

protocol/bc/txinput.go

+	return nil, false
+}
+
+func (t *TxInput) VMVer() uint64 {


Ditto here--is this used?

Nope, removed, thanks.

tessr · 2017-01-23T18:42:35Z

protocol/bc/issuance_witness.go

+}
+
+func (iw *IssuanceWitness) AssetDefinitionHash() (defhash Hash) {
+	if len(iw.AssetDefinition) == 0 {


Is this new if clause the reason that the merkle tree tests are changing?

No, this is just an optimization. The test vectors are changing because of the fixes to #330 and #335.

Hmm, I know we said we'd try to just merge this without teasing it apart more, but do you think it would be easy to pull those fixes out of this PR, and make this PR purely refactoring?

tessr · 2017-01-23T18:49:11Z

protocol/bc/spend.go

+
+// readCommitment reads a spend input commitment AFTER the leading
+// type byte has been consumed.
+func (si *SpendInput) readCommitment(r io.Reader, txVersion, assetVersion uint64) (err error) {


IMO it would be nice if SpendInput and IssuanceInput did the asset version check in the same way--right now, (IssuanceInput).readCommitment assumes that the asset version has been checked before it gets called, and (SpendInput).readCommitment does not

Do you mean you wish they had the same calling signature, maybe defined in an interface? The thing about that is, IssuanceInput commitments do not need the asset version right now, but SpendInput commitments do. We'd have to add unused parameters to the IssuanceInput version of readCommitment, which seems wrong in this case.

Would it be better if the two readCommitment functions had different names?

Perhaps a comment at the call site would be enough--I'm basically looking for ways to reduce cognitive load while reading this code.

tessr · 2017-01-23T19:05:18Z

protocol/bc/txoutput.go

+	return hash, err
+}
+
+// does not write the enclosing extensible string


might also add to this comment explaining why it's nil for now (same goes for readWitness)

Done, thanks.

tessr · 2017-01-23T19:06:58Z

protocol/bc/txinput.go

 	}
+	return assetID


Is this line reachable?

Well, I guess it is, if we get an input that's not an issuance or a spend. But what is assetID in this case?

Never mind, I just realized the return values are named. I'm still waking up today.

tessr · 2017-01-23T19:11:10Z

protocol/bc/txinput.go


+func (t *TxInput) ControlProgram() []byte {


Did (TxInput).ControlProgram() or (TxInput).IssuanceProgram() change?

You mean, with respect to main? No, they just got shuffled around a bit (sorry).

It's cool--github seems to be struggling a bit with the diff, just wanted to make sure I wasn't missing anything.

tessr · 2017-01-23T19:14:03Z

protocol/bc/txinput.go

-		return si.ControlProgram
+	if serflags&SerWitness != 0 {
+		_, err = blockchain.WriteExtensibleString(w, func(w io.Writer) error {
+			return t.writeInputWitness(w)


can t.WriteInputWitness be used as the param to WriteExtensibleString directly?

Yep, done, thanks.

tessr · 2017-01-23T19:18:00Z

protocol/bc/txinput.go

+			})
+			return err
+		}
+		return fmt.Errorf("unknown input type %T", t.TypedInput)


It seems like we sometimes return an error when we come across an unknown input type (like here, or in writeInputWitness), and sometimes we return a zero value (in AssetAmount or AssetID)--do we have a rule of thumb for this?

Not a conscious one, but your question made me think about the implicit one I've been using, which I think is: if the function doesn't already return an error value, don't add one for this purpose (which is the case for e.g. accessors); but if it does, then use it.

Not commenting on whether or not that's the right guideline to be using...

I feel like it's fine to say that accessors can return a zero value 👍

tessr · 2017-01-23T19:22:04Z

protocol/bc/transaction.go

+	return ew.Written(), ew.Err()
+}
+
+func (tx *TxData) writeTo(w io.Writer, serflags byte) error {


Did this function change?

With respect to main? Only in terms of comments and error wrapping - and the version on main is better. Updated.

bobg · 2017-01-23T20:03:29Z

Updated, PTAL

jbowens · 2017-01-26T01:11:33Z

looks like this might need a rebase?

bobg · 2017-01-29T20:22:33Z

!PTAL. This PR has gotten too stale, will redo it soon.

bobg added PTAL blockchain reset labels Dec 20, 2016

bobg force-pushed the txtypes branch from af3037e to 68997c3 Compare January 10, 2017 21:48

kr reviewed Jan 10, 2017

View reviewed changes

bobg pushed a commit that referenced this pull request Jan 11, 2017

Undo a bugfix, save it for #349.

a0ac943

bobg mentioned this pull request Jan 11, 2017

encoding/blockchain: Encapsulate "varstrlist" and "extensible string" patterns #376

Merged

bobg removed the PTAL label Jan 11, 2017

iampogo pushed a commit that referenced this pull request Jan 13, 2017

Undo a bugfix, save it for #349.

71d3308

bobg force-pushed the txtypes branch from 673c4e0 to 591d91d Compare January 13, 2017 20:51

bobg added the PTAL label Jan 13, 2017

tessr mentioned this pull request Jan 17, 2017

protocol/bc: better error messages when de/serializing transactions #393

Merged

iampogo pushed a commit that referenced this pull request Jan 17, 2017

protocol/bc: better error messages when de/serializing transactions

bb721ce

Part of #349. Closes #393

This was referenced Jan 17, 2017

protocol/bc: move SigHasher into its own file #394

Merged

protocol/bc: move AssetAmount into asset.go #395

Merged

iampogo pushed a commit that referenced this pull request Jan 17, 2017

protocol/bc: move SigHasher into its own file

b5feb4a

From #349. Closes #394

iampogo pushed a commit that referenced this pull request Jan 17, 2017

protocol/bc: move AssetAmount into asset.go

2c1b576

Part of #349 Closes #395

tessr mentioned this pull request Jan 17, 2017

protocol/bc: move OutputCommitment into its own file #397

Merged

tessr mentioned this pull request Jan 18, 2017

protocol/bc: return errors from WitnessHash functions #398

Merged

iampogo pushed a commit that referenced this pull request Jan 18, 2017

protocol/bc: return errors from WitnessHash functions

62d0c3a

This trickles down to `CalcMerkleRoot` as well. Part of #349. Closes #398

tessr mentioned this pull request Jan 18, 2017

protocol/bc: new files for TypedInputs #399

Merged

iampogo pushed a commit that referenced this pull request Jan 18, 2017

protocol/bc: new files for TypedInputs

db8f96c

Part of #349. In 349, there are other changes to both `IssuanceInput` and `SpendInput`, but I don't make those changes here. Closes #399

Bob Glickstein added 3 commits January 19, 2017 12:24

Shrink the diff by undoing a gratuitous function move.

612cb4a

Don't export Tx(Input|Output).witnessHash.

93035af

Post-merge tweaks.

2637230

bobg force-pushed the txtypes branch from 614450d to 2637230 Compare January 19, 2017 20:47

Why was the OutputCommitment type in charge of reading and writing th…

8bf7ddb

…e TxOutput witness??

tessr reviewed Jan 19, 2017

View reviewed changes

Bob Glickstein added 3 commits January 19, 2017 14:41

Unexport writeFastHash.

2126e29

Restore missing asset-version logic, reorganize exports.

f7953b6

POLA improvement.

bd891b4

tessr reviewed Jan 23, 2017

View reviewed changes

Address review comments.

ca0a377

bobg mentioned this pull request Jan 23, 2017

protocol/bc: fix tx sighash and witness hash bugs #423

Merged

bobg changed the title ~~protocol/bc: reorganize and refactor; also fix serialization errors~~ protocol/bc: reorganize and refactor Jan 24, 2017

bobg removed the blockchain reset label Jan 24, 2017

bobg removed the PTAL label Jan 29, 2017

bobg closed this Feb 13, 2017

bobg deleted the txtypes branch February 13, 2017 19:46

protocol/bc: reorganize and refactor #349

protocol/bc: reorganize and refactor #349

Conversation

bobg commented Dec 20, 2016 • edited

bobg commented Dec 20, 2016

bobg commented Jan 10, 2017

bobg commented Jan 10, 2017

kr commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobg commented Jan 11, 2017

bobg commented Jan 13, 2017

bobg commented Jan 17, 2017

tessr commented Jan 17, 2017

bobg commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tessr Jan 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobg commented Jan 23, 2017

jbowens commented Jan 26, 2017

bobg commented Jan 29, 2017

bobg commented Dec 20, 2016 •

edited

tessr Jan 23, 2017 •

edited