-
Notifications
You must be signed in to change notification settings - Fork 35
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
Blockchain updates for L2 extension #1430
base: master
Are you sure you want to change the base?
Conversation
pkg/state/transaction_handler.go
Outdated
@@ -264,6 +264,9 @@ func (h *transactionHandler) performTx( | |||
if err := snapshot.Apply(h.sa, tx, validatingUTX); err != nil { | |||
return txSnapshot{}, errors.Wrap(err, "failed to apply transaction snapshot") | |||
} | |||
|
|||
// write updates into the updatesChannel here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why and what changes exactly, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still relevant. I don't see any changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, the comment was left before I implemented the log, it's irrelevant now
pkg/state/appender.go
Outdated
|
||
// write updates into the updatesChannel here | ||
// TODO possibly run it in a goroutine? make sure goroutines run in order? | ||
if a.bUpdatesExtension != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having the dummy implementation of the "blockchain updates extension" looks simpler than ! = nil
checks, on my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends! For tests it was easier to send a nil, but maybe I should do what you suggested. How would you check if it's empty?
func(ctx context.Context, updatesChannel <-chan BUpdatesInfo) { | ||
for { | ||
select { | ||
case updates := <-updatesChannel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be wise to handle the closed channel case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for handling it, I saw you added it
pkg/state/accounts_data_storage.go
Outdated
@@ -242,7 +242,7 @@ func (s *accountsDataStorage) entryBytes(addr proto.Address, entryKey string) ([ | |||
func (s *accountsDataStorage) retrieveEntries(addr proto.Address) ([]proto.DataEntry, error) { | |||
addrNum, err := s.addrToNum(addr) | |||
if err != nil { | |||
return nil, err | |||
return nil, proto.ErrNotFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err
should be returned and wrapped with proto.ErrNotFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add check errors.Is(err, keyvalue.ErrNotFound)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be fixed
return L2ContractDataEntries{}, err | ||
} | ||
entry.SetKey(protoEntry.Key) | ||
switch e := entry.(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ProtobufConverter.Entry
method from pkg/proto/protobuf_converters.go
github.com/wavesplatform/gowaves/pkg/grpc/generated/waves/bupdates/blockchain_updates.pb.go
Outdated
Show resolved
Hide resolved
ctx context.Context, | ||
cfg *settings.BlockchainSettings, | ||
) (*state.BlockchainUpdatesExtension, error) { | ||
const l2ContractAddr = "3Msx4Aq69zWUKy4d1wyKnQ4ofzEDAfv5Ngf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter must be configurable.
586cc1e
to
5f1010c
Compare
pkg/state/appender.go
Outdated
Height: blockInfo.Height, | ||
}, | ||
} | ||
a.bUpdatesExtension.BUpdatesChannel <- bUpdatesInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation can block the whole node. Maybe add a timeout for writing the update?
pkg/state/accounts_data_storage.go
Outdated
@@ -242,7 +242,7 @@ func (s *accountsDataStorage) entryBytes(addr proto.Address, entryKey string) ([ | |||
func (s *accountsDataStorage) retrieveEntries(addr proto.Address) ([]proto.DataEntry, error) { | |||
addrNum, err := s.addrToNum(addr) | |||
if err != nil { | |||
return nil, err | |||
return nil, proto.ErrNotFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be fixed
/* Topics. */ | ||
const ( | ||
BlockUpdates = "block_topic" | ||
microblockUpdates = "microblock_topic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused
if entry.GetKey() == key { | ||
// Use a type switch to check the type | ||
switch entry.(type) { | ||
case *proto.BinaryDataEntry: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetValueType
method can be used to determine the exact data type of the entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestChangesGenerationNewEntries
and TestChangesGenerationContainsPrevious
are failing. Should be fixed.
ContractUpdatesInfo L2ContractDataEntries | ||
} | ||
|
||
// TODO wrap errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
} | ||
|
||
equal := true | ||
// todo REMOVE POINTERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
import "waves/block.proto"; | ||
|
||
|
||
message BlockInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think txCount
and Signature
fields have to be present in the message.
@@ -282,6 +282,79 @@ func (s *accountsDataStorage) retrieveEntries(addr proto.Address) ([]proto.DataE | |||
return entries, nil | |||
} | |||
|
|||
func (s *accountsDataStorage) retrieveEntriesAtHeight(addr proto.Address, height uint64) ([]proto.DataEntry, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually unused for now, because the caller stateManager.RetrieveEntriesAtHeight
is also unused.
pkg/state/appender.go
Outdated
|
||
// write updates into the updatesChannel here | ||
// TODO possibly run it in a goroutine? make sure goroutines run in order? | ||
if a.bUpdatesExtension != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think?
reader := bytes.NewReader(metaBlockValueBytes) | ||
|
||
// Extract blockHeight and epoch. | ||
readInt64(reader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readInt64(reader) | |
_ = readInt64(reader) |
} | ||
|
||
// Decode base64 and extract blockHeight and height. | ||
func extractEpochFromBlockMeta(metaBlockValueBytes []byte) int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to create a blockMeta
struct and UnmarshalBinary
method for it.
const StoreBlocksLimit = 200 | ||
const PortDefault = 4222 | ||
const HostDefault = "127.0.0.1" | ||
const ConnectionsTimeoutDefault = 5 * server.AUTH_TIMEOUT | ||
|
||
const NatsMaxPayloadSize int32 = 1024 * 1024 // 1 MB | ||
|
||
const PublisherWaitingTime = 100 * time.Millisecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these constants can be package private.
key := accountsDataStorKey{addrNum: addrNum} | ||
|
||
recordBytes, err := s.hs.entryDataAtHeight(key.bytes(), height) | ||
if errors.Is(err, keyvalue.ErrNotFound) || errors.Is(err, errEmptyHist) || recordBytes == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace recordBytes == nil
to len(recordBytes) == 0
.
} | ||
} | ||
|
||
equal := changes == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace changes == nil
to len(changes) == 0
return equal, changes, nil | ||
} | ||
|
||
func statesEqual(state BUpdatesExtensionState, scheme proto.Scheme) (bool, BUpdatesInfo, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a method from the function.
if err != nil { | ||
return false, nil, err | ||
} | ||
previousMap[dataEntry.GetKey()] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe cache the previous map?
currentState *BUpdatesInfo | ||
previousState *BUpdatesInfo // this information is what was just published |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe store info in a struct with BlockUpdatesInfo
and transformed map of data entries (key -> marshaled value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, replace all default log
calls in the new code to zap
logger calls.
Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.9.0 to 1.10.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.9.0...v1.10.0) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Return error instead of killing process with fatal log record.
Bumps [github.com/elliotchance/orderedmap/v2](https://github.com/elliotchance/orderedmap) from 2.4.0 to 2.5.0. - [Release notes](https://github.com/elliotchance/orderedmap/releases) - [Commits](elliotchance/orderedmap@v2.4.0...v2.5.0) --- updated-dependencies: - dependency-name: github.com/elliotchance/orderedmap/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Check on leasing state added. * Tests added. --------- Co-authored-by: Nikolay Eskov <[email protected]>
* Refactor and fix 'NewPeerInfoFromString' Add check for IP address parsing failure. Improved 'TestNewPeerInfoFromString'. * Improve 'NewPeerInfoFromString' Add hostname IP resolving. * Simplified 'NewTCPAddrFromString' * Refactoring and bugfix of 'resolveHostToIPv4' - Now it returns ipV4 address exactly in ipV4 form. - Fixed bug with skipping address for chesk after ipV6 removal. - Fixed bug with duplication of ipV4 address when resolving 'localhost'. * Optimized a bit 'filterToIPV4'. * Create 'NewPeerInfosFromString' function. * Use 'NewPeerInfosFromString' for resolving 'host:port' records by '-peers' CLI parameter. * Fix 'TestNewPeerInfoFromString' test. --------- Co-authored-by: Alexey Kiselev <[email protected]>
No description provided.