PegNet OPR Chain #40

WhoSoup · 2019-09-26T11:49:55Z

The biggest problem to fatd accepting non-FAT chains are the extids, which are checked at a very deep point in fatd, specifically this requirement tree:

For loading existing files:

chainmap.loadChains()
    db.OpenAll()
        db.Open(path)
            (db)chain.loadMetaData():
                There are three problems here
                a) fat.ValidTokenNameIDs (fails if it doesn't have exactly 4 extids)
                b) fat.TokenIssuer (requires at least 4 extids or it will panic)
                c) chain.setApplyFunc()
                    chain.applyIssuance
                        fat.NewIssuance().Validate() (checks extids for rcd pairs among other things)

For creating new chains:

chainmap.loadChains()
    (engine)chain.OpenNewByChainID()
        engine.OpenNew
            a) fat.ValidTokenNameIDs
            db.OpenNew()
                b) fat.ValidTokenNameIDs (again)
                c) fat.TokenIssuer
                d) chain.setApplyFunc() - see above

In order to load a pegnet chain into the system, I therefore have to either modify the parameters of at least loadMetaData() to specify a pegnet chain based on the "path", as well as db.OpenNew. That would scatter the PegNet code all over the system. To keep it all in the same place (chainmap.loadChains()), all the above functions would have to include parameters that selectively disable the features that will never work.

The second problem was that fatd's entry Apply() function was not aware of its position in the EBlock but Grading requires to know when the EBlock starts and when it ends. Further, fatd's processing of pending entries are applied directly without a block awareness.

Approach 1: https://github.com/WhoSoup/fatd/tree/pegnet-opr

I started at the top, and created a custom db.Open() and engine.OpenNew() for pegnet, which yielded a lot of code duplication. This let me just set the chain's type directly as well as set a custom database path (as suggested by adam at one point) for the pegnet sqlite files. The code duplication is a problem however, as less than a handful lines of code were the problem.

For the second problem, I created a "preEBlock" and "postEBlock" apply-function that, if set, is called during "ApplyEBlock". This leaves the Fat0/1 apply functions in place and the pending entries only use Apply but not ApplyEBlock so it had no impact.

Approach 2: (this pull request)

I learned a lot from doing approach 1 and decided to start at the bottom. I modified ValidTokenNameIDs and TokenIssuer to both accept the oprchain as a valid chain. TokenIssuer creates an identity for an entry with the hash 000..., since fatd accepts identity chains that are not yet created.

Thanks to this, I was able to simply add the OPR chain at the top of chain map and the system treated it mostly as a normal chain. For the second problem, I extended the Apply() function to become aware of its position, which I did by passing the eblock itself and the position of the entry inside the .Entries list to the Apply function. For pending entries, it passes an empty block and position -1.

Since our identity is blank the fatd system can't determine the issuance, which determines the chain's type, so I had to hardcode that to setApplyFunc(). I think a better solution may be to fake the Issuance as well but I haven't looked at that approach in-depth yet.

Things not yet added:

creating the coinbase transactions for payout
querying the rates table
api integration

WhoSoup · 2019-09-26T14:35:35Z

Just had an idea of using a specific fake chain for the identity, such as 00...001, to indicate that it's a pegnet identity. Then we can check chain.Identity.IsPegNet() instead of checking just if it's empty (which I assume has other uses in fatd). Thoughts?

AdamSLevy

Seeing this is very helpful to me to understand what the pegnet code will need. I'm going to continue to refactor the engine and db to allow for better of separation of code between fat and pegnet.

As a general rule, I'd rather see duplicated code than overly coupled code. It is easier to later deduplicate and to later decouple.

Since pegnet has no need to scan for new chains, the ValidTokenNameIDs function doesn't seem to be of much practical use to pegnet within the engine. If you want to fully validate your pegnet data structures, entries, name ids, etc, I'd like to see that done in a separate package from fat so that the intent is clear.

Additionally I'd like to see two Chain types that probably embed a shared Chain type with common variable and methods like Conn, Pool, Head EBlock. FAT chains and PegNet chains have different state related data that should be kept in memory for efficiency and convenience. Separate chain types for these things will allow that to be clearly expressed and not collide.

Any state related data should be kept in the chain type so that engine doesn't need to be bothered with passing it into the apply functions. that includes the latest (or latest few) EBlock(s) that your pegnet apply funcs might need to reference. That might include this pos variable as well, the index into the EBlock. But I'm still trying to understand the significance of that variable.

AdamSLevy · 2019-09-30T18:48:33Z

db/pegnet/grade.go

+	return nil
+}
+
+func GetGrade(conn *sqlite.Conn, seq uint32) ([]string, error) {


I would like to see the naming convention of SelectGrade be used here. An established convention in the factom package is that functions named Get... make network calls.

AdamSLevy · 2019-09-30T20:12:36Z

db/pegnet/grade.go

+		return nil, nil
+	}
+
+	buf := make([]byte, 2048)


It is possible to get the exact size of the buffer using Stmt.ColumnLen:

$ go doc sqlite.Stmt.ColumnLen package sqlite // import "crawshaw.io/sqlite" func (stmt *Stmt) ColumnLen(col int) int ColumnLen returns the number of bytes in a query result. Column indicies start at 0. https://www.sqlite.org/c3ref/column_blob.html

AdamSLevy · 2019-09-30T20:17:01Z

fat/type.go

@@ -32,6 +32,7 @@ type Type uint64
 const (
 	TypeFAT0 Type = iota
 	TypeFAT1
+	TypeFAT2


I hear that this type number is not really used anywhere. If that is the case then I see no need to have it. I don't want to constrain pegnet to exactly adhere to conventions that fat adopted that don't add a benefit to pegnet.

I used it to set the right apply function but if pegnet receives its own chain, it won't need it.

AdamSLevy · 2019-09-30T20:18:35Z

fat/issuance.go

@@ -89,6 +89,9 @@ func (i Issuance) MarshalJSON() ([]byte, error) {

 // NewIssuance returns an Issuance initialized with the given entry.
 func NewIssuance(entry factom.Entry) Issuance {
+	if IsPegNetOPR(entry.ExtIDs) {


Does pegnet really have an issuance entry at all?

If it does it's probably pretty different from this Issuance type. pegnet should probably just have its own data structures package that implements everything it needs independent of existing FAT types, except where there is a benefit to share.

AdamSLevy · 2019-09-30T20:19:46Z

fat/chainid.go

@@ -31,6 +31,9 @@ import (
 // ValidTokenNameIDs returns true if the nameIDs match the pattern for a valid
 // token chain.
 func ValidTokenNameIDs(nameIDs []factom.Bytes) bool {
+	if IsPegNetOPR(nameIDs) {


We should create two separate sets of functions. Ones for validating FAT token chains, and ones for validating pegnet chains. that will greatly improve the clarity of the code. The engine and db packages can stitch these things together as needed.

AdamSLevy · 2019-09-30T20:39:48Z

db/apply.go

+var tmp grader.BlockGrader
+
+func (chain *Chain) ApplyFAT2OPR(ei int64, e factom.Entry, eb factom.EBlock, pos int) (err error) {
+	if !eb.IsPopulated() || pos < 0 { // 'processing' entry


This really seems like it should return an error.

Also I believe it is a reasonable expectation that the caller has populated the EBlock. I think it would be best to not check if the eblock is populated and just let panics happen below here as this represents a serious integrity error with the code.

both "pending" entries and regular entries call the Apply function, but in the case of pending entries the entryblock passed into the function is nil and the position is -1 (unknown):

fatd/engine/process.go

Line 151 in 6d65886

if _, err := chain.Pending.Chain.ApplyEntry(e, factom.EBlock{}, -1); err != nil {

it's not an error because at the time it is called, the entry cannot be validated. it can only be validated in the context of an eblock

AdamSLevy · 2019-09-30T20:42:04Z

db/apply.go

+	// Every Entry
+	var extids [][]byte
+	for _, x := range e.ExtIDs {
+		extids = append(extids, []byte(x))


I see this is necessary for compatibility with the BlockGrader.AddOPR method. I'd rather see AddOPR be updated to accept []factom.Bytes, but for now this is ok.

However if you are going to do this, properly make extids to the correct size: len(e.ExtIDs), and then set the values instead of using append.

Also please follow the golang convention of using camel case everywhere and always capitalize both letters of ID, and use a descriptive variable name in the for loop instead of x.

extIDs := make([][]byte, len(e.ExtIDs) for i, extID := range e.ExtIDs { extIDs[i] = []byte(extID) }

I see this is necessary for compatibility with the BlockGrader.AddOPR method. I'd rather see AddOPR be updated to accept []factom.Bytes, but for now this is ok.

the grading module was written to be library independent

AdamSLevy · 2019-09-30T20:42:58Z

db/apply.go

@@ -234,6 +246,67 @@ func (chain *Chain) ApplyFAT0Tx(ei int64, e factom.Entry) (tx *fat0.Transaction,
 	return
 }

+var tmp grader.BlockGrader


this global is very very suspicious to me. Why can this not be created on the stack as its needed by functions?

This global seems more important than to be named tmp which tells me nothing about it.

this global is very very suspicious to me. Why can this not be created on the stack as its needed by functions?

it can be made a chain struct variable once there is a dedicated pegnet chain but that would surpass its scope.

the variable only needs to exist during the call of (db)chain.Apply() and would ideally be a local variable in there once the pegnet and fat chains are separated

AdamSLevy · 2019-09-30T20:53:27Z

db/apply.go

+	//fmt.Println(pos, len(eb.Entries))
+
+	// After every EBlock
+	if pos == len(eb.Entries)-1 {


this pos variable is concerning to me. This variable seems to be important, but it has a very terse name. I'd like a more descriptive name for it at least. Also it feels a lot like something that should be considered part of the state. As state, I think it should probably be a member variable of the future pegnet chain type. This may also be a way to keep the Apply function signatures succinct while still allowing state data to be passed through without the engine having to deal with it.

Also it feels a lot like something that should be considered part of the state.

"pos" is for position, which is the position of the entry being applied inside the eblock. it is the i counter variable of this function:

fatd/db/apply.go

Lines 63 to 68 in 6d65886

// Insert each entry and attempt to apply it...

for i, e := range eb.Entries {

if _, err = chain.ApplyEntry(e, eb, i); err != nil {

return

}

}

AdamSLevy · 2019-09-30T20:55:06Z

db/apply.go

+		extids = append(extids, []byte(x))
+	}
+
+	tmp.AddOPR(e.Hash[:], extids, []byte(e.Content))


later, if you'd let me, I'd like to review this code as well. At the minimum I may be able to suggest a number of things to improve memory usage.

later, if you'd let me, I'd like to review this code as well. At the minimum I may be able to suggest a number of things to improve memory usage.

sure but memory usage hasn't really been a problem. all the objects generated by the grader are ephemeral. they're discarded after a block is graded and only the relevant data is persisted

WhoSoup · 2019-10-08T09:23:14Z

Additionally I'd like to see two Chain types that probably embed a shared Chain type with common variable and methods like Conn, Pool, Head EBlock. FAT chains and PegNet chains have different state related data that should be kept in memory for efficiency and convenience. Separate chain types for these things will allow that to be clearly expressed and not collide.

I agree but that would involve a substantial modification to the existing code and that was outside the timeframe available at the time.

That might include this pos variable as well, the index into the EBlock. But I'm still trying to understand the significance of that variable.

Grading requires all entries to work and fatd doesn't include a way to check if an entry is the first/last entry inside an eblock. So either the apply function needs to be aware of which entry is the last entry (the pos flag), or there need to be special functions that run before / after eblocks (what i did in the first approach).

WhoSoup · 2019-10-08T10:05:21Z

after seeing your requests, i think the refactor of the engine is a necessity in order to start implementing pegnet

WhoSoup added 2 commits September 26, 2019 11:58

parse pegnet opr chain

66281b4

using go mod of branch

6d65886

AdamSLevy requested changes Sep 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PegNet OPR Chain #40

PegNet OPR Chain #40

WhoSoup commented Sep 26, 2019

WhoSoup commented Sep 26, 2019

AdamSLevy left a comment

AdamSLevy Sep 30, 2019

AdamSLevy Sep 30, 2019

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019

AdamSLevy Sep 30, 2019

AdamSLevy Sep 30, 2019

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019

AdamSLevy Sep 30, 2019

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019 •

edited

Loading

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019

AdamSLevy Sep 30, 2019

WhoSoup Oct 8, 2019

WhoSoup commented Oct 8, 2019

WhoSoup commented Oct 8, 2019

	// Insert each entry and attempt to apply it...
	for i, e := range eb.Entries {
	if _, err = chain.ApplyEntry(e, eb, i); err != nil {
	return
	}
	}

PegNet OPR Chain #40

Are you sure you want to change the base?

PegNet OPR Chain #40

Conversation

WhoSoup commented Sep 26, 2019

Approach 1: https://github.com/WhoSoup/fatd/tree/pegnet-opr

Approach 2: (this pull request)

WhoSoup commented Sep 26, 2019

AdamSLevy left a 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

WhoSoup Oct 8, 2019 • 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

WhoSoup commented Oct 8, 2019

WhoSoup commented Oct 8, 2019

WhoSoup Oct 8, 2019 •

edited

Loading