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

refactor: Integrate extracted and published near-account-id #10019

Merged
merged 28 commits into from
Nov 1, 2023

Conversation

ruseinov
Copy link
Contributor

@ruseinov ruseinov commented Oct 27, 2023

This PR supersedes #10001

Some context: #10001 (comment)

Part of: near/near-account-id-rs#5

@ruseinov ruseinov requested a review from a team as a code owner October 27, 2023 14:16
@ruseinov ruseinov requested a review from nikurt October 27, 2023 14:16
@ruseinov ruseinov marked this pull request as draft October 27, 2023 14:16
Cargo.toml Outdated Show resolved Hide resolved
chain/client/src/tests/bug_repros.rs Outdated Show resolved Hide resolved
@ruseinov ruseinov marked this pull request as ready for review October 27, 2023 20:16
@frol frol marked this pull request as draft October 27, 2023 20:25
integration-tests/src/node/mod.rs Outdated Show resolved Hide resolved
core/primitives/src/signable_message.rs Outdated Show resolved Hide resolved
runtime/runtime/src/balance_checker.rs Outdated Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Oct 27, 2023

@ruseinov Do NOT mark this PR ready for review until near-account-id 1.0 is released. This PR is not going to be merged with a git dependency.

@ruseinov
Copy link
Contributor Author

@ruseinov Do NOT mark this PR ready for review until near-account-id 1.0 is released. This PR is not going to be merged with a git dependency.

Sounds good!

@frol
Copy link
Collaborator

frol commented Oct 27, 2023

@ruseinov I see that CI still fails, please, take a closer look (I see that some of the recently merged PRs also had some CI checks failed, but fewer than in this PR)

@ruseinov
Copy link
Contributor Author

@ruseinov I see that CI still fails, please, take a closer look (I see that some of the recently merged PRs also had some CI checks failed, but fewer than in this PR)

Yep, some of those are confusing, looking into it.

@ruseinov ruseinov requested a review from frol October 28, 2023 23:28
@ruseinov
Copy link
Contributor Author

@frol I've made some fixes, but the workflows need to be approved every single time it seems :/ The upgradable stuff that fails seems to do so, because it's flaky. Sometimes it is green, sometimes it is not.

@frol
Copy link
Collaborator

frol commented Oct 29, 2023

The upgradable stuff that fails seems to do so, because it's flaky. Sometimes it is green, sometimes it is not.

I didn't see "upgradable" succeeding recently for this PR. I re-run it locally:

- label: "upgradable"
command: |
source ~/.cargo/env && set -eux
cd pytest
pip3 install --user -r requirements.txt
python3 tests/sanity/upgradable.py

And I get the following error in /tmp/near/upgradable/test1_finished/stderr and /tmp/near/upgradable/test2_finished/stderr (these are 1.35.0 release nodes and this test checks if protocol upgrade will be handled fine once transferred to the node that is built from this PR):

thread 'actix-rt|system:0|arbiter:2' panicked at chain/client/src/client.rs:1694:17:
Can't clear old data, Err(IOErr(Custom { kind: InvalidData, error: "Not all bytes read" }))
stack backtrace:
   0: rust_begin_unwind
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:595:5
   1: core::panicking::panic_fmt
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:67:14
   2: near_client::client::Client::on_block_accepted_with_optional_chunk_produce
             at /Users/frol/projects/near/nearcore/chain/client/src/client.rs:1694:17
   3: near_client::client::Client::postprocess_ready_blocks
             at /Users/frol/projects/near/nearcore/chain/client/src/client.rs:1418:13
   4: near_client::client_actor::ClientActor::try_process_unfinished_blocks
             at /Users/frol/projects/near/nearcore/chain/client/src/client_actor.rs:1210:13
   5: <near_client::client_actor::ClientActor as actix::handler::Handler<near_o11y::context::WithSpanContext<near_client::client_actor::ApplyChunksDoneMessage>>>::handle::{{closure}}
             at /Users/frol/projects/near/nearcore/chain/client/src/client_actor.rs:811:9
   6: near_performance_metrics::stats_disabled::measure_performance
             at /Users/frol/projects/near/nearcore/utils/near-performance-metrics/src/stats_disabled.rs:11:5
   7: <near_client::client_actor::ClientActor as actix::handler::Handler<near_o11y::context::WithSpanContext<near_client::client_actor::ApplyChunksDoneMessage>>>::handle
             at /Users/frol/projects/near/nearcore/chain/client/src/client_actor.rs:804:5
   8: <actix::address::envelope::SyncEnvelopeProxy<M> as actix::address::envelope::EnvelopeProxy<A>>::handle
             at /Users/frol/.cargo/registry/src/index.crates.io-6f17d22bba15001f/actix-0.13.0/src/address/envelope.rs:80:23
   9: <actix::address::envelope::Envelope<A> as actix::address::envelope::EnvelopeProxy<A>>::handle

That is borsh deserialization error that happens when there is more data than the struct needs to be fully deserialized. I cannot think of what can be wrong here, but it seems that the root cause is that the node from this PR serializes data that cannot be deserialized by 1.35.0 release. I still fail to see what could have gone wrong here.

@frol
Copy link
Collaborator

frol commented Oct 29, 2023

Hmmm, I tried running the test from master branch and I got exactly the same error, but it passed on CI somehow while CI fails for this PR.

@ruseinov
Copy link
Contributor Author

Hmmm, I tried running the test from master branch and I got exactly the same error, but it passed on CI somehow while CI fails for this PR.

Will check it out.

@frol
Copy link
Collaborator

frol commented Oct 31, 2023

@ruseinov It seems it was either a glitch or it has been fixed by some other PR merged to master somehow, but "upgradable" CI passes now.

@frol frol marked this pull request as ready for review October 31, 2023 18:45
@frol frol marked this pull request as draft October 31, 2023 18:48
@frol frol changed the title [Chore] Integrate external near-account-id. refactor: Integrate extracted and published near-account-id Oct 31, 2023
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

The change looks good to me, and it seems that the test that was blocking us is reproducible on master branch as well (the most recent commit to master failed with the same error as this PR used to, and this PR passes that test now with no changes made to address [or cause] that issue).

In order to avoid merge conflicts, I suggest we merge this change sooner rather later, so let's use 1.0.0-alpha.1 version, and once we update near-sdk-rs with it, I will just 1.0.0 release and submit a small PR bumping the version.

@frol frol marked this pull request as ready for review October 31, 2023 23:07
@frol
Copy link
Collaborator

frol commented Oct 31, 2023

@nikurt @nagisa Could you review and approve this PR or suggest someone who can do that? I believe this PR is ready to be merged.

@nikurt
Copy link
Contributor

nikurt commented Nov 1, 2023

@nikurt @nagisa Could you review and approve this PR or suggest someone who can do that? I believe this PR is ready to be merged.

The author should click the re-request button to re-request a review.

While we're at it, let me do more review begging. Please take a look at near/NEPs#514

Copy link
Contributor

@nikurt nikurt left a comment

Choose a reason for hiding this comment

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

Changes:

  • Replace near-account-id with an external crate
  • Simplify calls to AccountIdRef::as_ref()
  • Simplify parsing of AccountId

All of them look good and improve readability.

@@ -174,7 +172,7 @@ lru = "0.7.2"
memmap2 = "0.5"
memoffset = "0.8"
more-asserts = "0.2"
near-account-id = { path = "core/account-id", features = ["internal_unstable"] }
near-account-id = { version = "1.0.0-alpha.1", features = ["internal_unstable", "serde", "borsh"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

@frol frol added this pull request to the merge queue Nov 1, 2023
Merged via the queue into near:master with commit da8393b Nov 1, 2023
7 of 9 checks passed
@ruseinov ruseinov deleted the ru/chore/account-id-int-2 branch November 1, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants