Not really, if the genesisDoc wasn't set to begin with, we have to retrieve it from the DB.

We need to load the genesisDoc from the DB if unset hence the need for it.

The code you linked is in a function whose comment says that it is used in tests

I initially had it like that but in the description of the feature request @ebuchman says

We can provide methods on State to fetch them instead, and we can store them under special keys in the state database

which is why I do the fetches in the DB and store them respectively. Perhaps we could discuss the feature more and get a basic spec there, to come to consensus ;)

https://github.com/tendermint/tendermint/blob/develop/node/node.go#L141
https://github.com/tendermint/tendermint/blob/develop/state/state.go#L346

the comment needs to be deleted IMHO

Oh, I see. Forget everything I wrote then. I should be more careful about details

Need to remember not to do any reviews at night

Not a problem at all, they are great questions and better for simplicity, so no biggie.

second attempt :) what is the purpose of this? if we know that we set genesis doc only once during startup

IMHO that's coupling too much of the functionality into startup time, and as I had mentioned
the requirements of this package warrant us loading and setting the genesisDoc from and into the DB respectively. Because we are unexposing genesisDoc, SetGenesisDoc is the setter. However, I haven't yet seen any place in our code in which we change the GenesisDoc so you have a point here.

can be genesisDoc

Roger that, thanks.

Don't think that GenesisDoc() or ChainID() methods should return an error. If we know that genesis file is mandatory and TM will not work without it, maybe we can just throw a panic. That way we won't be needing to handle all these errors like https://github.com/tendermint/tendermint/pull/676/files#diff-80343e80650fe43a4394fc26afd5cb3bR508. I want to hear other opinions anyway.

That's a justified concern, it makes sense to simplify the PR more, which is nice. However, the requirements of the PR request that we instead have a way of alternatively loading the genesisDoc and chainID after loading them from a DB is what warrants us returning an error
since:

1. Non-existent data
2. Invalid data stored

Otherwise I'd have agreed that the PR can just be simplified to a single set and panic as you advise.

Agree, we should not return an error. This is a panic scenario.

what retr stands for? can't we just call it bytes or jsonBytes

retr --> retrieve*
Sure, I'll update that to your suggestion, thanks.

I think this comment should go into git commit msg

I'd rather keep it though as a mention for the future and purpose. Without it I feel like we'll be asking ourselves why we didn't just expose it as GenesisDoc, and also it is already mentioned in the git commit message :)

cmn

this assumes the genesis will always be available, even if we've already run the node. it's probably ok, but not sure we want to make that assumption

@jaekwon do you want to comment on this?

If we start a node, and then later the genesis file is changed, we will not be able to restart the node unless we can recover that genesis file. So I think it's worth having the genesis saved in the database to prevent a certain class of user errors. What do you think?

don't know. I am trying to come up with a scenario where it will be useful.

1. user loses state

then we'll need original genesis file to MakeGenesisState, but since we store all DBs together, a user will most likely lose genesis state too, so no point of storing it

2. user accidentally changes the genesis json (or some hacker changes it)

then TM will either fail to start due to a parsing error / missing genesis file error OR RPC will be serving incorrect genesis file. Is it worth saving it in the DB for the purpose of preventing such errors? I am not sure.

Date:   Fri Oct 13 13:11:16 2017 +0400

    save genesis doc in DB to prevent user errors

    https://github.com/tendermint/tendermint/pull/676#discussion_r144411458

─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: node/node.go
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ node.go:5 @ package node

import (
    "bytes"
    "encoding/json"
    "errors"
    "fmt"
    "net"
@ node.go:137 @ func NewNode(config *cfg.Config,
        return nil, err
    }

    genDoc, err := genesisDocProvider()
    // Get genesis doc
    genesisDB, err := dbProvider(&DBContext{"genesis", config})
    if err != nil {
        return nil, err
    }
    genDoc, err := loadGenesisDoc(genesisDB)
    if err != nil {
        genDoc, err = genesisDocProvider()
        if err != nil {
            return nil, err
        }
        // save genesis doc to prevent a certain class of user errors (e.g. when it
        // was changed, accidentally or not)
        err = saveGenesisDoc(genesisDB, genDoc)
        if err != nil {
            return nil, err
        }
    }

    state := sm.LoadState(stateDB)
    if state == nil {
@ node.go:554 @ func (n *Node) DialSeeds(seeds []string) error {
}

//------------------------------------------------------------------------------

var (
    genesisDocKey = []byte("genesisDoc")
)

func loadGenesisDoc(db dbm.DB) (*types.GenesisDoc, error) {
    bytes := db.Get(genesisDocKey)
    if len(bytes) == 0 {
        return nil, errors.New("Genesis doc not found")
    } else {
        var genDoc *types.GenesisDoc
        err := json.Unmarshal(bytes, &genDoc)
        return genDoc, err
    }
}

func saveGenesisDoc(db dbm.DB, genDoc *types.GenesisDoc) error {
    bytes, err := json.Marshal(genDoc)
    if err != nil {
        return err
    }
    db.SetSync(genesisDocKey, bytes)
    return nil
}

I wrote the code and can push it if we agree that we need to save genesis doc.

I'm not actually sure about passing GenesisDoc via a factory method (i.e. GenesisDocProvider). Almost by definition GenesisDoc creation is point-like and immutable, but a factory method implies it that it may need to created multiple times, lazily initialised, or icreated n different contexts that branch. None of these should be true for genesis. I think it would be better to make NewNode take a plain GenesisDoc as a argument (value type to emphasise the immutability.) Callers will just need to call your default provider rather than passing it. All I do for the provider is the busywork of wrapping my GenesisDoc in a func.

I also think that the GenesisDoc is an interesting part of the audit trail of the chain and you should probbly store it and serve it from state reliably.

Also in the world where we are handing provision of genesis doc and database provider to the caller we shouldn't make assumptions about what state is co-located or not or even stored to disk.

Come to think of it I actually think a hash of the GenesisDoc should be mixed into the initial AppHash. I don't think it's the same chain if it didn't start with the same GenesisDoc and validator set, but I suppose the way you have it now allows for some flexibility for the ABCI app in that respect (in next Burrow the initial AppHash is our GenesisDoc hash which includes validator set and time, ChainID is also derived from that)

wow, thanks for your input! ok, let's persist genesis doc

As for NewNode taking GenesisDoc as an argument, don't think this is what we want in the end. We call genesisDocProvider only once, then we save genesis doc to the state. If we rewrite the code to allow to pass it as an argument, this would mean the caller would have to retrieve it from some source on every start and that's not what we want.

what if we change the DBProvider to an interface:

type DBProvider interface{
  DB(*DBContext) (dbm.DB, error)
  GenesisDoc() GenesisDoc
}

then we can remove the genesisDocProvider, and folks can wrap up their strategies for genesis files and dbs however they want

meh, this is kinda weird.

Ok I think what we currently have is fine. Let's let it lie for a bit until something genuinely better emerges.

right - we shouldn't have a need for this (but I see this is why the genesis was moved out of the if block. The chainID should be saved somewhere. The ConsensusParams may also change over the life of the chain (not implemented yet), so we don't want to set them back to the genesis params every time we start

see above

I know Jae doesn't like private mutable data fields in the State. We may need to seek another way to do this ... perhaps the params need to live in the ConsensusState.

Immutable private chainID should be ok.

perhaps the params need to live in the ConsensusState.

Will these params always be for consensus only (maybe we'll want to add blockchain params or something?).

Also, if I am not mistaken, we do not persist ConsensusState (as we recreate it using blockstore & WAL). So, how about we store ChainID & Params under a separate key (genesisParamsKey)?

Ok, I think we should just expose ChainID and Params in here and let them be written every time too.

I'd ask @jaekwon to confirm.

Also if we will want historical params like we have historical validators once we update ABCI to allow changes there.

It would also be amazing if state encoding was extensible so we dont have a breaking change every time we want to change something here!

Yeah we should definitely expose ChainID.
ConsensusParams is small enough, we can just write it.

see above

eh. Maybe it's fine if ChainID is written to disk every time - it's small, and might be worth avoiding this extra step of needing to set it. I can see folks easily forgetting to call this, and everything will probably break without it.

++

I prefer defer s.mtx.Unlock() unless there's something to do after unlocking.

Do we need the pointer ?

GenesisDoc uses a pointer value https://github.com/tendermint/tendermint/pull/676/files/2eba353e2ed3da4c1ae18b4bde3e7f853a7a66a3#diff-f6be34387a141f652301c16cf7a5e36cL272 so that's just transferring the field in here. However, to answer your question, usually I'll go with pointers because then I can find out if something was set or not idiomatically with

if g.Params == nil {
}

as opposed to

if g.Params == types.ConsensusParams{} {
}

or

if reflect.DeepEqual(g.Params, types.ConsensusParams{}) {
}

I think it depends on how often params will be changing. If not very often, we can make it a raw struct. If the opposite, then making it a pointer makes sense for faster updates.

we can remove this comment now

If we start a node, and then later the genesis file is changed, we will not be able to restart the node unless we can recover that genesis file. So I think it's worth having the genesis saved in the database to prevent a certain class of user errors. What do you think?

maybe we panic here actually?

you're right

also the double pointer here can be confusing. Isn't it better to:

genDoc := new(types.GenesisDoc)
Unmarshal(bytes, genDoc)

?

mm.. using *smth seems more standard. I am not used to using new