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: delayed finalization #214

Merged
merged 3 commits into from
Nov 29, 2023
Merged

feat: delayed finalization #214

merged 3 commits into from
Nov 29, 2023

Conversation

evilrobot-01
Copy link
Contributor

Adds --finalize-delay-sec cli argument, based on the great work by @shunsukew at inkdevhub/swanky-node#61.

Manual testing by starting node with/without option, along with example contract upload:

# build example contract
cargo contract build --manifest-path=../ink-examples/erc20/Cargo.toml

# start node
cargo run

# upload contract
cargo contract upload --suri //Alice --execute --manifest-path=../ink-examples/erc20/Cargo.toml
# check finalized head remains at genesis
sleep 1
test $(curl -sH "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "chainHead_unstable_genesisHash", "params":[]}' http://localhost:9944 | jq .result) \
  = $(curl -sH "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "chain_getFinalizedHead", "params":[]}' http://localhost:9944 | jq .result) && echo PASS || echo FAIL


# start node (with delayed finalization)
cargo run -- --finalize-delay-sec 1

# upload contract
cargo contract upload --suri //Alice --execute --manifest-path=../ink-examples/erc20/Cargo.toml
# check finalized head matches chain head
sleep 1
test $(curl -sH "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "chain_getHead", "params":[]}' http://localhost:9944 | jq .result) \
  = $(curl -sH "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "chain_getFinalizedHead", "params":[]}' http://localhost:9944 | jq .result) && echo PASS || echo FAIL

Closes #160

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Thanks a lot. We have to keep in mind that the default behaviour changes now from "never finalize" to "finalize right away".

I think it would be better to change the default value so something like 2 seconds instead of 0. It is a more sane default as it forces client devs to think about those two states when working with the dev node. Instead of assuming they are the same.

cc @wottpal

@evilrobot-01
Copy link
Contributor Author

It is currently opt-in only by specifying the --finalize-delay-sec option, so the value is left up to the user. The default behaviour should therefore remain the same for any existing use cases unless explicitly specified, and manually specifying it should force the developer to think about the additional state. I can see how having it as a default might be useful, but I thought it better to start with it as optional.

Perhaps the notes in the command description/readme around using 'a value of 0 for instant finalization' should simply be replaced with a suggestion of 2 seconds? The command example in the readme suggests 5 seconds, so I can standardise all of the documentation to 2 seconds.

@wottpal
Copy link

wottpal commented Nov 28, 2023

thanks for the heads up, @athei. really appreciate that this is coming to substrate-contracts-node finally. 🚀 no strong opinion about making it the default or not. but if yes, something like 1s would be totally sufficient to make frontend devs aware of handling the additional state IMO.

@athei
Copy link
Member

athei commented Nov 28, 2023

I did misread the code. Didn't notice it was an Option. So yes indeed the default behaviour does not change with this PR. This is okay as it is non invasive.

However, I don't think having blocks to be never finalized is of any use (current behaviour). I suggest removing the Option and just set the default to 1 as @wottpal suggested. It should still be okay as we only add finalization. e2e tests listen for block inclusion and shouldn't be slowed down by this.

@evilrobot-01
Copy link
Contributor Author

Updated as suggested, where it now defaults to delayed finalization of 1 second but still allows developer to override.

Note that the first manual test in the original PR comment above no longer applies, as that was testing the default method remained unchanged.

@evilrobot-01 evilrobot-01 requested a review from athei November 29, 2023 11:58
@athei athei merged commit 813d99f into main Nov 29, 2023
6 checks passed
@athei athei deleted the frank/feat-delayed-finalize branch November 29, 2023 13:23
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.

Provide a feature to specify block finality time
3 participants