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

Wiring ForkedChainRef to other components #2423

Merged
merged 22 commits into from
Sep 4, 2024
Merged

Wiring ForkedChainRef to other components #2423

merged 22 commits into from
Sep 4, 2024

Conversation

jangko
Copy link
Contributor

@jangko jangko commented Jun 27, 2024

  • Disable majority of hive simulators
  • Only enable pyspec_sim for the moment
  • The pyspec_sim is using a smaller RPC service wired to ForkedChainRef
  • The RPC service will gradually grow

tersec
tersec reviewed Jun 29, 2024
View reviewed changes
nimbus/rpc/server_api.nim Outdated Show resolved Hide resolved
tersec
tersec reviewed Jun 29, 2024
View reviewed changes
nimbus/rpc/server_api.nim Outdated Show resolved Hide resolved
@arnetheduck
Copy link
Member

I think let's work on this in a branch until hive and/or other disabled testing works again

@jangko
Copy link
Contributor Author

jangko commented Jun 30, 2024

in hive we have 5 test suites: rpc, graphql, engine-api, consensus, and pyspec.

rpc suite and graphql are not completely related to ForkedChain. They have their own domain of problem.
Only engine-api suite and pyspec have direct dependancy to ForkedChain.
Consensus test suite although using the same test vectors as test_blockchain_json, It can be fixed separately from this branch because it's entry point is from rlp block import.

@jangko jangko mentioned this pull request Jul 3, 2024
Merged
@jangko
Wiring ForkedChainRef to other components
ead8cb6 
- Disable majority of hive simulators
- Only enable pyspec_sim for the moment
- The pyspec_sim is using a smaller RPC service wired to ForkedChainRef
- The RPC service will gradually grow
@jangko
Addressing PR review
9997a27
@jangko
Fix test_beacon/setup_env
8947736
@jangko
Enable consensus_sim (#2441)
e5099b4 
* Enable consensus_sim

* Remove isFile check
advaita-saha
advaita-saha reviewed Aug 27, 2024
View reviewed changes
nimbus/db/core_db/core_apps.nim Outdated
Comment on lines 343 to 349
db.ctx.getKvt().hasKey(genericHashKey(blockHash).toOpenArray).isOkOr:
if error.error != KvtNotFound:
warn logTxt info, blockHash, action="hasKey()", error=($$error)
return false

return true

Copy link
Contributor

Choose a reason for hiding this comment

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

hasKey() doesn't return error if blockHash is not found. So with this current implementation of hasBlockHeader, all unknown block headers are getting ignored as it thinks it is already there in the db

nimbus-eth1/nimbus/db/kvt/kvt_utils.nim

Lines 136 to 154 in 8bf581e

proc hasKey*(
db: KvtDbRef; # Database
key: openArray[byte]; # Key of database record
): Result[bool,KvtError] =
## For the argument `key` return the associated value preferably from the
## top layer, or the database otherwise.
##
if key.len == 0:
return err(KeyInvalid)
if db.layersHasKey key:
return ok(true)
let rc = db.getBe key
if rc.isOk:
return ok(true)
if rc.error == GetNotFound:
return ok(false)
err(rc.error)

@jangko jangko marked this pull request as ready for review September 3, 2024 03:23
@tersec tersec mentioned this pull request Sep 3, 2024
Open
@tersec tersec merged commit 4d9e288 into master Sep 4, 2024
26 checks passed
@tersec tersec deleted the server-api branch September 4, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tersec tersec tersec left review comments

@advaita-saha advaita-saha advaita-saha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jangko @arnetheduck @tersec @advaita-saha