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

Blockchain updates for L2 extension #1430

Open
wants to merge 60 commits into
base: master
Choose a base branch
from
Open

Conversation

esuwu
Copy link
Contributor

@esuwu esuwu commented Jun 14, 2024

No description provided.

@esuwu esuwu changed the title Initial draft Initial design draft Jun 14, 2024
@@ -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
Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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


// write updates into the updatesChannel here
// TODO possibly run it in a goroutine? make sure goroutines run in order?
if a.bUpdatesExtension != nil {
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think?

Copy link
Contributor Author

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:
Copy link
Member

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.

Copy link
Contributor Author

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/appender.go Fixed Show fixed Hide fixed
@esuwu esuwu changed the title Initial design draft Blockchain updates for L2 extension Jul 3, 2024
@@ -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
Copy link
Member

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

Copy link
Member

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)

Copy link
Member

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) {
Copy link
Member

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

@nickeskov nickeskov added wip This is a WIP, should not be merged right away do not merge The PR is not ready to be merged labels Aug 26, 2024
ctx context.Context,
cfg *settings.BlockchainSettings,
) (*state.BlockchainUpdatesExtension, error) {
const l2ContractAddr = "3Msx4Aq69zWUKy4d1wyKnQ4ofzEDAfv5Ngf"
Copy link
Member

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.

@nickeskov nickeskov force-pushed the node-updates-plugin-l2 branch from 586cc1e to 5f1010c Compare November 7, 2024 03:58
Height: blockInfo.Height,
},
}
a.bUpdatesExtension.BUpdatesChannel <- bUpdatesInfo
Copy link
Member

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?

@@ -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
Copy link
Member

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"
Copy link
Member

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:
Copy link
Member

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.

Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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 {
Copy link
Member

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) {
Copy link
Member

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.


// write updates into the updatesChannel here
// TODO possibly run it in a goroutine? make sure goroutines run in order?
if a.bUpdatesExtension != nil {
Copy link
Member

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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
readInt64(reader)
_ = readInt64(reader)

}

// Decode base64 and extract blockHeight and height.
func extractEpochFromBlockMeta(metaBlockValueBytes []byte) int64 {
Copy link
Member

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.

Comment on lines +13 to +20
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
Copy link
Member

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 {
Copy link
Member

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
Copy link
Member

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) {
Copy link
Member

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.

pkg/blockchaininfo/nats_publisher.go Show resolved Hide resolved
if err != nil {
return false, nil, err
}
previousMap[dataEntry.GetKey()] = value
Copy link
Member

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?

Comment on lines +29 to +30
currentState *BUpdatesInfo
previousState *BUpdatesInfo // this information is what was just published
Copy link
Member

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)?

Copy link
Member

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.

nickeskov and others added 15 commits November 18, 2024 07:37
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge The PR is not ready to be merged wip This is a WIP, should not be merged right away
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants