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

Provide a feature to specify block finality time #160

Closed
DoubleOTheven opened this issue Nov 13, 2022 · 13 comments · Fixed by #214
Closed

Provide a feature to specify block finality time #160

DoubleOTheven opened this issue Nov 13, 2022 · 13 comments · Fixed by #214

Comments

@DoubleOTheven
Copy link

instant finality makes front end development difficult, and unrealistic. It would be great to be able to simulate the various states of a transaction in block, finalized, etc

@wottpal
Copy link

wottpal commented Feb 22, 2023

Hey, did you found a solution, @DoubleOTheven?

I'm facing a similar problem right now. Not only from the perspective that frontend development is more artificial then (no loading states etc.), but I also have the problem that polkadot.js/api transactions never yield an isFinalized state (isInBlock is always the latest) which I think is caused by the instant finality since #42.

Small example:

await tx.signAndSend(account, (result) => {
      const isInBlock = result?.status?.isInBlock    // works on all nodes
      const isFinalized = result?.status?.isFinalized  // never `true` on `substrate-contracts-node` but on all others
})

Any ideas @cmichi @jacogr? Do I miss something?

@athei
Copy link
Member

athei commented Feb 22, 2023

I think we should just move to a very short block time (<< 1s) instead of having instant finality.

@wottpal
Copy link

wottpal commented Feb 22, 2023

I think we should just move to a very short block time (<< 1s) instead of having instant finality.

Would be perfectly fine from my side (if that actually solves the problem above, but I guess) :)

@athei
Copy link
Member

athei commented Feb 22, 2023

The main concern from @cmichi was that it will slow down e2e tests against the node. But if we make the block time really short it should be no problem. I think we can go as low as we want since no networking is involved.

wdyt @cmichi ?

@cmichi
Copy link
Contributor

cmichi commented Feb 24, 2023

We could also add a flag --instant for e2e tests and default to a block time of e.g. 0.5 sec (to have some orientation on what is available to parachains).

@cmichi
Copy link
Contributor

cmichi commented Feb 24, 2023

@PierreOssun Do you have any thoughts here? And more important: would you by chance be up for implementing it 😊? Asking because you were so kind to do the original PR for instant finality.

@wottpal
Copy link

wottpal commented Apr 6, 2023

Just pushing this up once again, as I stumble across issues everywhere due to the instant finality nature of substrate-contracts-node.

For example I wanted to index my local node with Subsquid, and when starting its archive (https://github.com/subsquid/substrate-archive-setup) it says chain height: 0. But when connecting to a different local node (i.e. aleph-node) it works instantly. Already spoke to one of their devs who also assumes this is related to instant block finality.

@shunsukew
Copy link

shunsukew commented Apr 21, 2023

@cmichi How about this approach?
If this seems good, I'll continue to make PR in the Substrate repo.
spawning new task which subscribes block import stream, and finalizes it after waiting for configured delay time.
https://github.com/shunsukew/substrate/pull/1/files

Same as spawning two tasks for e.g. aura (authoring) & grandpa (finality),
Nodes need to start two tasks in service.rs, run_instant_seal and run_delayed_finalize.

Now, delay_sec is set to 5 as hard coded. But, it is possible to make it configurable.
set to 0 -> instant finality
set to 1 -> 1 second finality after sealing a block

@shunsukew
Copy link

shunsukew commented Apr 24, 2023

@wottpal
I'm preparing PR in Substrate, but it may take long time. you can use this swanky-node's branch if you wish inkdevhub/swanky-node#61, if approved I'm going to make swanky-node's release.

@shunsukew
Copy link

Released v1.6.0 with delayed finalize feature
https://github.com/AstarNetwork/swanky-node/releases/tag/v1.6.0

@wottpal
Copy link

wottpal commented Apr 27, 2023

Thanks @shunsukew, I just tried it, and it's working smoothly 👌🥳

It fixes both problems mentioned above:

  • polkadot.js extrinsic status now gets finalized,
  • local subsquid archive node is working as expected and recognizes block height (FYI @RaekwonIII).

Would love to see this in substrate-contracts-node, too @cmichi @athei 🤩

@shunsukew
Copy link

That's great to hear!

@athei
Copy link
Member

athei commented Apr 28, 2023

Would love to see this in substrate-contracts-node, too @cmichi @athei 🤩

PRs welcome :)

athei pushed a commit that referenced this issue Nov 29, 2023
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:
```shell
# 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
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 a pull request may close this issue.

5 participants