Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implemented basic block construction #401

Merged
merged 6 commits into from Jan 7, 2019
Merged

Conversation

antonlerner
Copy link
Contributor

No description provided.

origin common.Address,
destination common.Address,
amount *big.Int) *Transaction{
return &Transaction{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation

@@ -0,0 +1,23 @@
package common
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this package include only min functions, maybe rename it to min or math ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a task

HareResult : t.hareRes.GetResult(),
Orphans:t.orphans.GetOrphans(),
}
copy(b.Txs, txs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just assign txs to Txs?

network:net,
weakCoinToss:weakCoin,
orphans:orph,
started: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use an atomic int or lock a mutex. unless you don't expect Start Stop and others to be threadsafe

log.Error("cannot serialize block %v", err)
return
}
t.network.Broadcast(meshSync.BlockProtocol,bytes)
Copy link
Contributor

@y0sher y0sher Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also related to @almogdepaz , usually here other impls of blockchains(!), broadcast an announcement including the hash of the block so peers would know they can use sync to receive it from us. some kind of push-pull gossip protocol

}()

case tx := <- t.newTrans:
t.transactionQueue = append(t.transactionQueue,Transaction2SerializableTransaction(tx))
Copy link
Contributor

@y0sher y0sher Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its nice to put that in a select loop but we've already discussed mutexes are more efficient when projecting state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a more flexible code, channels can be consumed from arious producers if we decide to, and will eliminate the need for functions that need to be locked with this mutex. also, if we decide the channel receiving txs should be buffered it will be more efficient than simple locking a mutex when adding a single value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be efficient as it will be non blocking but that doesn't mean it'll get txs to the cache faster. keep in mind that every part of the select might block this from happening even if the channel has txs in it.

Coin bool
Timestamp time.Time
Txs []SerializableTransaction
HareResult []mesh.BlockID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Votes? also shouldn't this be two slices for positive and negative result ? or a pair with voting direction and +/-.
also I don't see anything about view edges here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

believe this one is solved as per @almogdepaz comment below

Coin bool
Timestamp time.Time
Txs []SerializableTransaction
HareResult []mesh.BlockID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better names
hareresult=VotePattern
orphans=view

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I didn't get that orphans are actually the view edges at first.

hareResult:make(chan HareResult),
transactionQueue:make([]SerializableTransaction,0,10),
hareRes:nil,
mu:sync.Mutex{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't have to do this, in go mutex's nil value is a working mutex. when initializing structs without names, you must specify every field. so that makes you must init even nil values but that's not the case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a functional reason not to initialize a value explicitly?

AccountNonce uint64
Price []byte
GasLimit uint64
Recipient *common.Address
Copy link
Contributor

@almogdepaz almogdepaz Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would move address out of common

Origin: origin,
Recipient:&destination,
Amount: amount,
GasLimit:10,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for Price.
also why hash is nil ?


case id := <-t.beginRoundEvent:
txList := t.transactionQueue[:common.Min(len(t.transactionQueue), MaxTransactionsPerBlock)]
t.transactionQueue = t.transactionQueue[common.Min(len(t.transactionQueue), MaxTransactionsPerBlock):]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like no coinbase tx is added here. also nothing mentioned about collecting fees

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no mining fees nor transaction fees are included in this milestone and this PR as well

return fmt.Errorf("BlockBuilderStopped")
}
tx := state.NewTransaction(nonce,origin,destination,amount)
t.newTrans <- tx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can make this a oneliner with the line about

return fmt.Errorf("BlockBuilderStopped")
}
tx := state.NewTransaction(nonce,origin,destination,amount)
t.newTrans <- tx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newTras might block clients of AddTransaction atleast note it as a comment.


func (t *BlockBuilder) createBlock(id mesh.LayerID, txs []SerializableTransaction) Block {
b := Block{
Id : mesh.BlockID(rand.Int63()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block id is just rand num ? no block hash ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, block id is more of a nonce, block hash is the hash of the entire block including the nonce

started bool
}

type Block struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually that is separated to header and block body. are we planning to do so ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. but not now

Recipient:&destination,
Amount: amount,
GasLimit:10,
Price:big.NewInt(1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if we don't know what gaslimit and price are right now, maybe it would be nicer to pass them to this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@antonlerner antonlerner merged commit 82d4b30 into develop Jan 7, 2019
@y0sher y0sher deleted the transaction_box branch January 14, 2019 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants