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

Wasmer Signal Propagation #495

Closed
yun-yeo opened this issue Jul 27, 2020 · 10 comments · Fixed by #504
Closed

Wasmer Signal Propagation #495

yun-yeo opened this issue Jul 27, 2020 · 10 comments · Fixed by #504
Labels
Milestone

Comments

@yun-yeo
Copy link

yun-yeo commented Jul 27, 2020

follow: CosmWasm/wasmd#4

@ethanfrey seems Wasmer reborn will be not ready soon and they will not make new release including @hanjukim's solution. Can you fork Wasmer including that solution? and make production ready CosmWasm with forked Wasmer?

@ethanfrey
Copy link
Member

I talked to them a week ago and they said they will have better signal handling and plan to release on a similar scale to our 1.0 release (in 4-6 weeks).

I would stay with that until that proves unfeasible.

I have run testnets and restarted nodes with the current solution and it is not a blocker. I agree it is good to improve before mainnet and if wasmer is much slower than expected I will review this again later on. But this should not impact any testnet or mainnet prep

@yun-yeo
Copy link
Author

yun-yeo commented Jul 27, 2020

ok cool thanks @ethanfrey

@hanjukim
Copy link

hanjukim commented Jul 27, 2020

We don't need wasmer-reborn, we just want signal error to be fixed. We need this to be fixed before we release Terra Columbus-4 and if my patch for wasmer cannot by applied to official cosmwasm release any time soon, we will have to fork everything which is a little bit inconvenient.

@syrusakbary
Copy link

@hanjukim we can provide a new release with the patch included :)

Let me talk tomorrow with the team

@ethanfrey
Copy link
Member

@syrusakbary We are using 0.17.0, as we cannot use 0.17.1 (it breaks gas metering in our case where we reload the singlepass-compiled module from disk). If the new release builds on 0.17.1 we cannot use it either unless that test passes. (I think that regression was not addressed as it was fixed in wasmer-reborn).

@hanjukim I do not understand why this blocks your testnet. Regen ran a 70 validator testnet with the same behavior and had no major issues. (It is annoying in shutdown and can cause broken db in some rare cases... a few nodes had to resync, but the chain never halted. Also I think the db behavior is much better in 0.39 than 0.38)

@hanjukim
Copy link

hanjukim commented Jul 29, 2020

@ethanfrey I just don't want that rare cases to be happened in software.. I don't think it's good to rely on luck even though it can be fixed. I think graceful shutdown is essential function for production level softwares.

@webmaster128
Copy link
Member

and can cause broken db in some rare cases

If this is the case, I'd blame the DB. A DB must not corrupt data when killed at any time (which can happen at a power loss).

@ethanfrey
Copy link
Member

If this is the case, I'd blame the DB. A DB must not corrupt data when killed at any time (which can happen at a power loss).

Yup, a database knows no such thing as graceful shutdown. It should handle the laptop thrown against a wall and recover. I know the lead dev of https://crates.io/crates/sled and he fuzz tests all kind of disk corruption issues and how to auto-recover. But yeah, some db in cosmos-sdk doesn't seem so robust (not sure if it is the iavl store or the tendermint block store)

@webmaster128
Copy link
Member

We'll not work on this on the unmaintained 0.17.x version of Wasmer. Instead we can look into this again once #503 is done.

@webmaster128 webmaster128 added this to the 1.0.0 milestone Sep 7, 2020
@webmaster128
Copy link
Member

According to wasmerio/wasmer#842 (comment) and wasmerio/wasmer#1496 (comment) this is done in Wasmer Reborn.

Is there a way to test this directly in this repo without a chain?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants