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

Secret Connection network timeouts #2

Closed
tarcieri opened this issue Aug 1, 2019 · 6 comments
Closed

Secret Connection network timeouts #2

tarcieri opened this issue Aug 1, 2019 · 6 comments

Comments

@tarcieri
Copy link
Contributor

tarcieri commented Aug 1, 2019

Copying/pasting this from the original issue at tendermint/tmkms#310

The Secret Connection implementation presently has no notion of timeouts for things like connecting/"dialing" validators or reading from/writing to sockets.

Without timeouts, there are many cases which could potentially deadlock indefinitely inside of blocking I/O operations, and at least one user has experienced this.

Here are a few recommendations:

  • futures/tokio: this is the up-and-coming ecosystem solution to this general problem, and the one I'd recommend. It's a full asynchronous event model which solves, among other things, timeouts. When async/await support ships in Rust 1.38 (scheduled to be released in August), migrating from blocking I/O should be fairly straightforward.
  • libc crate + poll(2) system call: if we wanted to stick with blocking I/O, the poll(2) system call, as invoked through the libc crate, can be used to determine I/O readiness prior to performing a blocking I/O call, and also takes a timeout as an argument. This would probably be the lowest impact way to implement timeouts as it wouldn't involve switching away from blocking I/O.

I would suggest migrating to futures-rs/Tokio to solve this problem, and leveraging the async/await syntax when it lands later this month.

@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 9, 2019

If anyone wants to take a crack at switching Secret Connection to use Tokio, they have just migrated to std::future upstream and released an alpha that supports it:

https://tokio.rs/blog/2019-08-alphas/

Using it will require nightly (soon to be 1.38 beta) Rust

@tarcieri
Copy link
Contributor Author

Another option (announced yesterday) is async-std, which is a sort of drop-in async replacement for std::io:

It allows easily adding timeouts which leverage futures while using an API that looks nearly identical to std::io (with the addition of async/await):

async fn get() -> io::Result<Vec<u8>> {
    let mut stream = TcpStream::connect("example.com:80").await?;
    stream.write_all(b"GET /index.html HTTP/1.0\r\n\r\n").await?;

    let mut buf = vec![];

    io::timeout(Duration::from_secs(5), async {
        stream.read_to_end(&mut buf).await?
        Ok(buf)
    })
}

@liamsi
Copy link
Member

liamsi commented Nov 8, 2019

@xla want to pair up on this soon?

@tarcieri
Copy link
Contributor Author

tarcieri commented Nov 9, 2019

@liamsi FWIW I copied Secret Connection back to the KMS repo with the goal of working on async support over there (see #27)

tomtau pushed a commit to yihuang/tendermint-rs that referenced this issue Nov 19, 2019
* build(deps): update tai64 requirement from 2 to 3 (informalsystems#22)

Updates the requirements on [tai64](https://github.com/iqlusioninc/crates) to permit the latest version.
- [Release notes](https://github.com/iqlusioninc/crates/releases)
- [Commits](iqlusioninc/crates@canonical-path/v2.0.0...tai64/3.0.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* max_gas should be i64, can be -1

* remove unneed clones

* remove secret_connection

* implement utility traits for tendermint data types
@ebuchman
Copy link
Member

Can we close this here then?

@liamsi
Copy link
Member

liamsi commented Dec 11, 2019

Yes, doesn't make sense to have it open here. There used to be an open issue there tendermint/tmkms#310 but this is also tracked by tendermint/tmkms#352.

@liamsi liamsi closed this as completed Dec 11, 2019
@greg-szabo greg-szabo mentioned this issue Oct 6, 2020
5 tasks
greg-szabo added a commit that referenced this issue Oct 6, 2020
ebuchman pushed a commit that referenced this issue Oct 8, 2020
* Removed amino traits, block, signature, vote

* tendermint: Fix header hashing

This commit fixes the header hashing mechanism to match that of the
current Go version. It also adds a test to ensure the happy path works.

Signed-off-by: Thane Thomson <[email protected]>

* light-client: Resolve #600

Demonstrates that the previous commit (fixing the header hash) fixes #600
when executed against a running Tendermint node.

Signed-off-by: Thane Thomson <[email protected]>

* tendermint: Use shorthand form of method call

Signed-off-by: Thane Thomson <[email protected]>

* tendermint: Add header hash check in integration test

Signed-off-by: Thane Thomson <[email protected]>

* tendermint: Add workaround for empty BlockId

Prost seems to encode an empty raw protobuf BlockId as an empty vector
of bytes, whereas the Go encoder encodes the same object to [18, 0].
This commit adds a workaround to demonstrate that this is the case when
testing against Tendermint v0.34 (otherwise header hashing would fail).

Signed-off-by: Thane Thomson <[email protected]>

* rpc: Update integration test fixture

It seems as though the previous commit data was generated by an older
version of Tendermint. The new commit fixtures here are generated by
Tendermint 0.34.0-99aea7b0 running in a Docker container on my local
machine.

Signed-off-by: Thane Thomson <[email protected]>

* Added Proposal domain type with signing - tests fail for now

* tendermint: Make last_block_id encoding workaround more explicit

Signed-off-by: Thane Thomson <[email protected]>

* tendermint: Fix empty last_block_id encoding issue

Signed-off-by: Thane Thomson <[email protected]>

* Proposal and Vote signable bytes functions

* PubKeyResponse made into PublicKey

* Header.Hash() fixes

* Merge conflict fixes

* Proposal test fixes

* Updated CHANGELOG

* Update CHANGELOG.md

* Update CHANGELOG.md

* regenerate static model-based files

* Update tendermint/Cargo.toml

* Remvoed amino bech32 encoding

* Updated priv_validator.json test with non-amino public key result

* Removed comments referencing amino.

* Review fixes #1

* Review fixes #2

* Review fixes #3

* Updated tests using testgen (#615)

* Update tendermint/src/public_key.rs

Co-authored-by: Tomas Tauber <[email protected]>

* fmt fix

* Fixes number four, additional derive traits

Co-authored-by: Thane Thomson <[email protected]>
Co-authored-by: Andrey Kuprianov <[email protected]>
Co-authored-by: Tomas Tauber <[email protected]>
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

No branches or pull requests

3 participants