Skip to content

Block store - #42

Merged
tzdybal merged 7 commits into
mainfrom
tzdybal/blockstore
Apr 30, 2021
Merged

Block store#42
tzdybal merged 7 commits into
mainfrom
tzdybal/blockstore

Conversation

@tzdybal

@tzdybaltzdybal commented Apr 21, 2021

Copy link
Copy Markdown
Contributor

This PR introduces Block store.

Depends on: #40, #41.

@liamsiliamsi mentioned this pull request Apr 21, 2021
Merged
@tzdybal
tzdybalforce-pushed the tzdybal/addr_conversion branch from 611ba4d to 40d3c2fCompareApril 21, 2021 10:36
@tzdybal
tzdybal marked this pull request as ready for review April 21, 2021 13:47
@tzdybal
tzdybalforce-pushed the tzdybal/blockstore branch from 99a1705 to 2ad7280CompareApril 22, 2021 08:33
@tzdybal
tzdybalforce-pushed the tzdybal/blockstore branch from 2ad7280 to d93c7baCompareApril 22, 2021 08:40
@tzdybal
tzdybal requested review from Wondertan and liamsi and removed request for WondertanApril 22, 2021 08:53
@tzdybal
tzdybalforce-pushed the tzdybal/blockstore branch from 622366d to 2f19fd6CompareApril 22, 2021 09:10
Wondertan
Wondertan reviewed Apr 22, 2021
View reviewed changes
Comment threadp2p/client_test.go
Comment on lines 70 to 71
func TestBootstrapping(t *testing.T) {
log.SetLogLevel("dht", "INFO")
_ = log.SetLogLevel("dht", "INFO")
//log.SetDebugLogging()

@WondertanWondertanApr 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if we need to keep that logging/debugging code in master actually. I usually remove that code before merging and put it back for local usage only without committing

Comment threadstore/blockstore.go
Comment threadstore/blockstore.go Outdated
Comment threadstore/blockstore.go
}

func (bs *DefaultBlockStore) SaveBlock(block *types.Block) error {
// TODO(tzdybal): proper serialization & hashing

@WondertanWondertanApr 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not make it proper already?
If we building things from scratch here, sticking with easy and performant solutions like sha256-simd from the beginning makes sense.

@tzdybaltzdybalApr 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Goal of this PR is to introduce block store that works. Using sha256-simd actually makes sense as it's drop-in replacement. By this comment I mostly mean that the serialization and hashing code will definitely be modified in the future.

@Wondertan

Wondertan commented Apr 22, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

@tzdybal, @liamsi, how full ORU blocks are going to be transported between ORU nodes, both aggregators, and simple users and is there a use case for that?

@liamsi

liamsi commented Apr 22, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

how full ORU blocks are going to be transported between ORU nodes

That is not specified yet. It will be different from lazyledger-core for sure. If you want to write an ADR for that, that would be more than welcome. But remember that after an aggregator proposes a block, its data will also be available on LL and can be downloaded by anyone once this feature lands: https://github.com/lazyledger/lazyledger-core/issues/221 (cc @Wondertan).
So the default could be just that.

This was referenced Apr 25, 2021
Closed
Closed
@tzdybal
tzdybal changed the base branch from tzdybal/addr_conversion to mainApril 26, 2021 13:23
liamsi
liamsi reviewed Apr 28, 2021
View reviewed changes
Comment threadrpcclient/local.go
Comment threadstore/blockstore.go
Comment threadstore/blockstore.go Outdated
Comment threadstore/blockstore.go
Comment threadstore/blockstore.go Outdated
Comment threadstore/blockstore.go
Comment on lines +85 to +87
// TODO(tzdybal): any better way to convert slice to array?
var h [32]byte
copy(h[:], hash)

@liamsiliamsiApr 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that is the most common way. You could add a helper method (or methods) if you have to convert back and forth.

Comment threadstore/blockstore.go Outdated
Comment threadstore/blockstore.go
return &block, nil
}

// TODO(tzdybal): replace with proper hashing mechanism

@liamsiliamsiApr 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be good to decide on all cryptographic primitives in ADR I think.

Comment threadstore/blockstore_test.go Outdated
Comment threadstore/kv.go
@tzdybal
tzdybal requested review from Wondertan and liamsiApril 30, 2021 08:12
liamsi
liamsi reviewed Apr 30, 2021
View reviewed changes
Comment threadstore/blockstore_test.go
getRandomBlock(4, 100),
getRandomBlock(5, 10),
getRandomBlock(1, 10),
}},

@liamsiliamsiApr 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much better!

liamsi
liamsi approved these changes Apr 30, 2021
View reviewed changes

@liamsiliamsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tzdybal
tzdybal merged commit bc1ca00 into mainApr 30, 2021
@liamsi
liamsi deleted the tzdybal/blockstore branch April 30, 2021 17:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamsiliamsiliamsi approved these changes
@WondertanWondertanAwaiting requested review from Wondertan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tzdybal@Wondertan@liamsi