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 71 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
pkg/state/appender.go Outdated Show resolved Hide resolved
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

Copy link
Member

@nickeskov nickeskov Jan 17, 2025

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) and only then error should be wrapped

@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
nickeskov and others added 24 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]>
#1569)

* Pulled the contract updates from snapshots, removed by block filtering

* Fixed a wrong check

* Removed temporary fix

* Added the first block processing

* Added an error handling

* Fixed a linter issue
pkg/blockchaininfo/nats_publisher.go Outdated Show resolved Hide resolved
cmd/blockchaininfo/nats_subscriber.go Outdated Show resolved Hide resolved
pkg/blockchaininfo/nats_publisher.go Outdated Show resolved Hide resolved
pkg/blockchaininfo/bupdates.go Outdated Show resolved Hide resolved
Comment on lines +2367 to +2369
if IsNotFound(err) {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary?

"github.com/wavesplatform/gowaves/pkg/proto"
)

const natsTestURL = "nats://127.0.0.1:4756"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe choose a free port for testing purposes?

Comment on lines +113 to +115
func ConcatenateContractTopics(contractAddress string) string {
return blockchaininfo.ContractUpdates + contractAddress
}
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate

Comment on lines +130 to +133
syncErr := l.Sync()
if syncErr != nil {
log.Fatalf("failed to sync zap logger %v", syncErr)
}
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 not necessary to use Fatal log level here. I suggest to use Error or Warn level here.

Comment on lines +102 to +106
go func() {
if err := printContractInfo(contractUpdatesInfo, scheme, path); err != nil {
zap.S().Errorf("Failed to print contract info updates: %v", err)
}
}()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use goroutine here? Maybe use WaitGroup to manage goroutines resources after SIGINT receival?

if pblshErr != nil {
zap.S().Errorf("failed to publish changes, %v", pblshErr)
}
bu.previousState = &updates
Copy link
Member

Choose a reason for hiding this comment

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

Updates can not replace the WHOLE previous state. As I can see updates are snapshots from the last block (after the first block is sent).
How is the whole logic work? Can you explain me, please?

Comment on lines +13 to +14
const EpochKeyPrefix = "epoch_"
const blockMeta0xKeyPrefix = "block_0x"
Copy link
Member

Choose a reason for hiding this comment

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

Can be package private.

Comment on lines +101 to +105
value, err := dataEntry.MarshalValue()
if err != nil {
return false, nil, err
}
currentMap[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.

Why do you store data entry in serialized form when you can store objects?

Comment on lines +161 to +167
n, err := reader.Read(buf)
if err != nil {
return nil, err
}
if n != length {
return nil, errors.Errorf("expected to read %d bytes, but read %d bytes", length, n)
}
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaced by io.ReadFull

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add testcase with blockchain rollback scenario?

@esuwu
Copy link
Contributor Author

esuwu commented Jan 17, 2025

  • data entries patch
  • mutex for extensionState
  • move writing into channel

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