Release 0.16 - alignment with consolidated tooling wrt chain making #501

benjaminbollen · 2017-02-19T23:52:51Z

After consolidation of eris-cm and eris-pm into eris tooling; some technical debt had to be resolved. This PR is necessary for this re-alignment in eris https://github.com/eris-ltd/eris/pull/1235 to complete.

checklist:

clean up eris-db dependencies of permissions; merged in Cleanup: introduce word256 and remove dependency on tendermint/go-common #499
permissions utility to convert map[string]int into permissions type
genesis: define constructors for Genesis types to contain explicit dependency on tendermint/go-crypto into tooling; enabling us to inject our own crypto package without coordination with tooling.
genesis: introduce GenesisPrivateValidtor as an intermediary step to removing priv_validator logic from the stack
genesis: provide genesis.GenerateGenesisFileBytes(chainName, []*GenesisAccount, []*GenesisValidator) for tooling
final review

…nto AccountPermission

…unt and GenesisValidator

silasdavis

Looks good in terms of structure which is definitely an improvement.

I take issue with the over-defensive use of cloning everything. Often you are cloning values that are copied on the call stack anyway. Where you are dealing with pointers it ought really be part of the contract that we are not handed mutable bytes by the implementer. In terms of the actual implementations we can see that we are not passed mutable bytes and we know we are not mutating them. So I'm not sure the extra complexity is justified. Plus it signals that we need to be cloning we I am not convinced we do. Though perhaps I am missing something.

silasdavis · 2017-02-21T19:59:32Z

genesis/genesis.go

+
+	// TODO: assert valid accounts and validators
+	// TODO: [ben] expose setting global permissions
+	globalPermissions := ptypes.DefaultAccountPermissions.Clone()


I don't think this is necessary DefaultAccountPermissions is a struct value so it's copied on assignment.

Yes, but is a struct AccountPermissions of a struct BasePermissions and a slice of strings; I believe - but not sure - that the struct BasePermissions is treated as a pointer essentially; but definitely the Roles []string is an effective pointer;

secondly, genesis is currently constructed with Params as a pointer to a GenesisParams which is a pointer to an AccountPermissions; I dont particularly like this construction; but I didnt want to end up with a GenesisDoc that didnt own its own data;

Arguably this does read very defensive

The inner struct is just another value, the memory is copied in a single pass. See: https://play.golang.org/p/Ut_7gZ3TyJ

Yeah the slice does give you mutable access. But this is go, I think we need to choose our battles. I'd rather we were passing immutable sequences of values but we're not. And in the event we are not writing back to the roles, and that would be a weird thing to do.

silasdavis · 2017-02-21T20:03:44Z

genesis/make_genesis_file.go

@@ -28,6 +28,25 @@ func GenerateKnown(chainID, accountsPathCSV, validatorsPathCSV string) (string,
 }

 //------------------------------------------------------------------------------------
+// interface functions that are consumed by monax tooling
+
+func GenerateGenesisFileBytes(chainName string, genesisAccounts []*GenesisAccount,


Similar to other comment, if it's convenient to pass genesisAccounts []GenesisAccount then we could probably get away without a clone... (though we do have pointers from GenesisAccount to AccountPermissions, but I don't think we accidentally mutate here)

in tooling there is a usage of passing slices of pointers; which is justified there because they are passed around a lot during construction;

this is why i deliberately gave tooling an interface that respects the [ ]*T; but GenesisDoc is constructed with [ ]T and once GenesisDoc is constructed changing the original argument T should not affect the GenesisDoc

thought that might be the case. Makes sense.

silasdavis · 2017-02-21T20:05:19Z

genesis/maker.go

+	switch keyType {
+	case "ed25519":
+		// TODO: [ben] functionality and checks need to be inherit in the type
+		if len(publicKeyBytes) != 32 {


I would rather replace all magic number references with named constants

yes, but tendermint/go-crypto/pub_key.go does not have this definition; and while I agree with you. I prefer to hold off making that definition until (very soon) we make eris-db/crypto

We should just name it in the same file, let's not make the perfect an enemy of the good.

silasdavis · 2017-02-21T20:05:41Z

genesis/maker.go

+				len(publicKeyBytes))
+		}
+		// ed25519 has type byte 0x01
+		typedPublicKeyBytes = make([]byte, 33)


likewise would prefer publicKeyLength + 1

yes, on the same page, but without a const, 32+1 is a bit weird

yep, which is why you should introduce the constant. In the same file if necessary.

Im ok with introducing the constants and moving them later

silasdavis · 2017-02-21T20:06:01Z

genesis/maker.go

+		// prepend type byte to public key
+		typedPublicKeyBytes = append([]byte{crypto.PubKeyTypeEd25519}, publicKeyBytes...)
+	case "secp256k1":
+		if len(publicKeyBytes) != 64 {


silasdavis · 2017-02-21T20:06:09Z

genesis/maker.go

+				len(publicKeyBytes))
+		}
+		// secp256k1 has type byte 0x02
+		typedPublicKeyBytes = make([]byte, 65)


silasdavis · 2017-02-21T20:09:33Z

genesis/types.go

+	}
+	// clone the public key by writing and reading over go-wire serialisation
+	// TODO! write unit test to see whether this is correct
+	publicKeyClone, err := crypto.PubKeyFromBytes(genesisValidator.PubKey.Bytes())


I can see how this is sort of defensive. But I wonder if it is a step too far. Ultimately we have to trust that we are not being handed internal bytes slice that we might mutate. We can see for sure that we are not being handed a reference to internal bytes as it is.

Having a Clone in general is very good practice, because it allows you to make it explicit when you're copying data; and as a result avoid copying it when you don't have to.

Here again Clone is introduced, because on construction of the genesisDoc I want to have unique ownership of GenesisDoc

I do get it, it's just using non-pointer values is idiomatic way of doing this in go. If this is really such a special case that we need to be extra explicit about copying then maybe. But my concern is that fighting the language like this leads to a mess/a burden we cannot hope to (would not want to) live up to everywhere.

I particularly don't like this given that we are running it through go-wire for little reason.

Im not sure whether it is fighting the language or fighting bad habits

using go-wire was here rather elegant; otherwise it requires many lines of reflecting which keytype it is then copying it in the correct fixed size byte array and then casting it back to the interface;

in my opinion interface PubKey should have a Clone() but it does not; so using the available interfaces worked quite nicely

because it allows you to make it explicit when you're copying data; and as a result avoid copying it when you don't have to.

You are copying data exactly when you don't have to because PubKey.Bytes() does a copy itself (as it ought to)

but it returns a []byte as type byte prepended public key bytes; I need to then copy that byte slice in a [32]byte or [64]byte and then cast that to the interface; this one line captures all that;

Im fine for it to be improved in eris-db/crypto; but right now I need a way to take deep ownership of the data in genesisDoc so that once the GenesisDoc is created it is independent of the memory that went into creating it

My point is you have a PubKey, you don't need a clone, it's the same PubKey genesisValidator.PubKey and the PubKey interface is immutable.

And you already own the memory because you created it: https://github.com/benjaminbollen/eris-db/blob/441c1a6db5b3d679b94df5bd0335ee32f5d9697e/genesis/maker.go#L77

…Silas

…cutout-genesis

benjaminbollen · 2017-02-27T20:15:04Z

continued in #506

…se-0.16-cutout-genesis Release 0.16 - alignment with consolidated tooling wrt chain making

benjaminbollen added 2 commits February 16, 2017 13:27

permissions: introduce utility to convert string map of permissions i…

8ccff1f

…nto AccountPermission

genesis: add GenesisPrivateValidator and constructors for GenesisAcco…

14ddd4d

…unt and GenesisValidator

benjaminbollen added status/dont merge type/technical debt labels Feb 19, 2017

benjaminbollen added 5 commits February 20, 2017 17:25

genesis: rework genesis

3270d0c

eris-mint: remove lingering genesis/types

aaf27e9

genesis: MakeGenesisDocFromAccountsAndValidators

804cadc

genesis: add GenerateGenesisFileBytes()

1e71bd4

genesis: corrections to clone and other minor fixes

4c1951d

benjaminbollen force-pushed the release-0.16-cutout-genesis branch from b8c93cb to 4c1951d Compare February 21, 2017 17:40

benjaminbollen removed the status/dont merge label Feb 21, 2017

silasdavis suggested changes Feb 21, 2017

View reviewed changes

benjaminbollen added 2 commits February 21, 2017 22:11

genesis: add const for public key length

441c1a6

genesis: remove redundant copy of public key bytes on clone - thx to …

578e434

…Silas

silasdavis approved these changes Feb 22, 2017

View reviewed changes

Merge remote-tracking branch 'origin/release-0.16' into release-0.16-…

fa0ed7e

…cutout-genesis

silasdavis merged commit cc2ce6a into hyperledger-archives:release-0.16 Feb 22, 2017

silasdavis added a commit to silasdavis/burrow that referenced this pull request Mar 9, 2019

Merge pull request hyperledger-archives#501 from benjaminbollen/relea…

a6f0922

…se-0.16-cutout-genesis Release 0.16 - alignment with consolidated tooling wrt chain making

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 0.16 - alignment with consolidated tooling wrt chain making #501

Release 0.16 - alignment with consolidated tooling wrt chain making #501

benjaminbollen commented Feb 19, 2017 •

edited

Loading

silasdavis left a comment

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017 •

edited

Loading

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017 •

edited

Loading

silasdavis Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

silasdavis Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

benjaminbollen Feb 21, 2017

silasdavis Feb 21, 2017

silasdavis Feb 21, 2017 •

edited

Loading

benjaminbollen commented Feb 27, 2017

Release 0.16 - alignment with consolidated tooling wrt chain making #501

Release 0.16 - alignment with consolidated tooling wrt chain making #501

Conversation

benjaminbollen commented Feb 19, 2017 • edited Loading

silasdavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silasdavis Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen Feb 21, 2017 • edited Loading

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

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

silasdavis Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

benjaminbollen commented Feb 27, 2017

benjaminbollen commented Feb 19, 2017 •

edited

Loading

silasdavis Feb 21, 2017 •

edited

Loading

benjaminbollen Feb 21, 2017 •

edited

Loading

silasdavis Feb 21, 2017 •

edited

Loading