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

feat: add --stateless flag to forest #3593

Merged
merged 48 commits into from
Oct 30, 2023
Merged

feat: add --stateless flag to forest #3593

merged 48 commits into from
Oct 30, 2023

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Oct 16, 2023

Summary of changes

As part of #3556

Changes introduced in this pull request:

  • Add a stateless mode to Forest. Forest still connects to the p2p swarm in this mode but does not sync to HEAD.
  • For Hello requests, respond with the heaviest tipset in the database(genesis by default).
  • If a block is requested that we do not have, forward the request to other peers and cache the result. We don't forward the request as discussed here
  • Respond to ChainRequests with PartialResponse. We never want to respond with more than 1 block. Retrieve from the network when it's not available locally.
  • CI test

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review October 20, 2023 15:34
@hanabi1224 hanabi1224 requested a review from a team as a code owner October 20, 2023 15:34
@hanabi1224 hanabi1224 requested review from lemmih and LesnyRumcajs and removed request for a team October 20, 2023 15:34
@@ -65,6 +65,8 @@

- [#3422](https://github.com/ChainSafe/forest/issues/3422) Add NV21 (Watermelon)
support for calibration network.
- [#3593](https://github.com/ChainSafe/forest/pull/3593): Add `--stateless` flag
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 worth elaborating a bit on what this flag exactly does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


STATELESS_NODE_ADDRESS=$($FOREST_CLI_PATH net listen | tail -n 1)
echo "Stateless node address: $STATELESS_NODE_ADDRESS"
STATELESS_NODE_PEER_ID=$(echo "$STATELESS_NODE_ADDRESS" | cut --delimiter="/" --fields=7 --zero-terminated)
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth commenting on an expected format with an example multiaddress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

EOF

# Disable discovery to not connect to more nodes
$FOREST_PATH --chain calibnet --encrypt-keystore false --auto-download-snapshot --config "$CONFIG_PATH" --rpc false --mdns false --kademlia false --metrics-address 127.0.0.1:6117 &
Copy link
Member

Choose a reason for hiding this comment

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

Could we disable mdns and kademlia in config? Unless you think passing it as flags makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've no preference, fixed as suggested


source "$(dirname "$0")/harness.sh"

forest_init_stateless
Copy link
Member

Choose a reason for hiding this comment

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

Will this node be killed in case of an error somewhere along the way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the detached node will be killed by trap forest_cleanup EXIT in harness.sh

Copy link
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

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

Looks good.

I'm not sure it'll actually work but there's no way of testing this without just deploying it and seeing if anything breaks.

@hanabi1224 hanabi1224 added this pull request to the merge queue Oct 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2023
@hanabi1224 hanabi1224 added this pull request to the merge queue Oct 30, 2023
Merged via the queue into main with commit 952a63e Oct 30, 2023
26 checks passed
@hanabi1224 hanabi1224 deleted the hm/stateless-mode branch October 30, 2023 13:10
LesnyRumcajs added a commit that referenced this pull request Nov 1, 2023
LesnyRumcajs added a commit that referenced this pull request Dec 13, 2023
LesnyRumcajs added a commit that referenced this pull request Dec 13, 2023
LesnyRumcajs added a commit that referenced this pull request Dec 14, 2023
LesnyRumcajs added a commit that referenced this pull request Dec 14, 2023
@hanabi1224 hanabi1224 mentioned this pull request Jan 1, 2024
4 tasks
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.

None yet

3 participants