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.

Sign up for GitHub

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

Jump to bottom

Update neo-go version #50

Merged
merged 2 commits into from
Dec 16, 2020
Merged

Update neo-go version #50

merged 2 commits into from
Dec 16, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Dec 8, 2020

Related #49 .
Need to wait until preview4 to update C# nodes.

@roman-khimov
Copy link
Member

Need to wait until preview4 to update C# nodes.

I don't think so, they should work fine with latest NEP-17 PRs merged. The trick here is exactly to make it all work as of current state of both nodes (and they're relatively synchronized at the moment).

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 8, 2020

@roman-khimov the problem is that C# docker file requires release tarball, and I haven't found if built for master.

@roman-khimov
Copy link
Member

See Dockerfile.sharp.sources.from_binaries (and Dockerfile.sharp.sources.from_local_dependencies, though that might be more involved, it's OK to have just the first one for now).

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 9, 2020

There are two unmerged PRs neo-project/neo-node#681 neo-project/neo#2083 . Because of incompatible container fails to be built. I'll try using git instead of downloading zip files.

@roman-khimov
Copy link
Member

Just leave neo-vm at compatible commit, it hasn't been changed functionally for quite some time and we certainly don't need .NET 5.0 at the moment.

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 11, 2020

Updated neo to commit right before introduction of management contract and neo-vm right before dotnet 5.0 update. Everything builds, though nodes accepted by C# node are empty. Nothing is logged, currently investigating.

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 11, 2020 •
edited
Loading

Both nodes can be built and used in bench. But C# node accepts only 512 tx per block. Seems like policy contract invoke somehow failed. Tested via start.*Single10wrk.

roman-khimov
roman-khimov reviewed Dec 11, 2020
View reviewed changes
.docker/ir/docker-compose.single.sharp.yml Outdated
@@ -6,7 +6,7 @@ services:
- stats
image: registry.nspcc.ru/neo-bench/neo-sharp:bench
logging:
driver: "none"
driver: "journald"
Copy link
Member

Choose a reason for hiding this comment

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

NACK.

Makefile
@@ -13,9 +13,9 @@ DC_SHARP_RPC=.docker/rpc/docker-compose.sharp.yml

DF_GO=.docker/build/Dockerfile.golang
DF_BENCH=.docker/build/Dockerfile.bench
DF_SHARP=.docker/build/Dockerfile.sharp
#DF_SHARP=.docker/build/Dockerfile.sharp
#DF_SHARP=.docker/build/Dockerfile.sharp.sources.from_binaries
Copy link
Member

Choose a reason for hiding this comment

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

I thought this one is easier to make work actually.

@roman-khimov
Copy link
Member

But C# node accepts only 512 tx per block.

Well, why is that?

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 14, 2020

The problem is in invalid policy contract hash (on a commit previous to management contract introduction).

@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 14, 2020

The problem was in contract hashes. Commit for neo-go with fix is from nspcc-dev/neo-go#1616 . Another thing is increased cost of transfer (TBD fix in neo-go).

@AnnaShaleva AnnaShaleva mentioned this pull request Dec 15, 2020
Merged
@fyrchik
Copy link
Contributor Author

fyrchik commented Dec 16, 2020

Updated neo-go and neo to master, neo-vm is still one commit behind .NET update.

roman-khimov
roman-khimov approved these changes Dec 16, 2020
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

If all the logic changes needed are in place, we'd rather merge it as is to unblock #52. Changing neo-go revision is rather trivial.

@roman-khimov roman-khimov merged commit 2a5227e into master Dec 16, 2020
@roman-khimov roman-khimov deleted the update branch December 16, 2020 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

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
@fyrchik @roman-khimov @AnnaShaleva