Merge crate abci-rs into abci #112

devashishdxt · 2020-01-10T03:30:24Z

Merge abci-rs into abci which uses latest async/await feature of Rust.

Fixes #115
Fixes #107
Fixes #105
Fixes #30

tomtau

I'd prefer this to be done in a non-breaking / less drastic way, e.g.:

abci-rs functionality is exposed under an optional module
the existing rust-abci trait functionality is still there, but marked as deprecated

devashishdxt · 2020-01-10T05:45:05Z

I'd prefer this to be done in a non-breaking / less drastic way, e.g.:

abci-rs functionality is exposed under an optional module

the existing rust-abci trait functionality is still there, but marked as deprecated

What do you mean by optional module? Also, people who want to use existing rust-abci traits can use current version. We can release this as a major version change (0.7.0) signifying non-backwards compatible changes.

tomtau · 2020-01-10T06:12:46Z

I'd prefer this to be done in a non-breaking / less drastic way, e.g.:

abci-rs functionality is exposed under an optional module

the existing rust-abci trait functionality is still there, but marked as deprecated

What do you mean by optional module? Also, people who want to use existing rust-abci traits can use current version. We can release this as a major version change (0.7.0) signifying non-backwards compatible changes.

e.g. that there's a module next.rs (or any name) that exposes that functionality and can be enabled with some feature-flag. My preference for that is that there's a transitional period during which the old Rust abci server has some limited support before it's removed -- but if the consensus is for an immediate major version change switch, I'm fine with it

tac0turtle · 2020-01-10T11:08:27Z

I was thinking if this is such a breaking change could it wait for Tendermint 0.33 ( i am working on finalizing it) and then this will start with support for the new version, as it holds many breaking changes.

Is this needed for users that are currently on the 0.32 series

tomtau · 2020-01-11T08:58:00Z

Is this needed for users that are currently on the 0.32 series

the new code adds, for example, the communication over unix domain sockets... so not needed, but good to have

src/server.rs

examples/counter.rs

devashishdxt · 2020-01-20T07:30:43Z

@marbar3778 Updated PR to work with Tendermint v0.33.0

src/types.rs

tac0turtle · 2020-01-22T13:48:32Z

could a changelog entry be added with a short synopsis of the changes? also if you have a twitter we'd love to give you a shoutout for this work.

devashishdxt · 2020-01-22T14:53:41Z

@marbar3778 I don’t have a Twitter account. You can link my GitHub account if you want. :)

liamsi

Great work! Very clean and well done PR. I was wondering how extensive this was tested? Agree with @marbar3778 that a proper changelog entry would be awesome!

@tomtau @yihuang are there still any major concerns about this being too breaking?

devashishdxt · 2020-01-24T02:46:40Z

@marbar3778 @liamsi: Added changelog entry

tomtau

@tomtau @yihuang are there still any major concerns about this being too breaking?

0.33 is breaking anyway, so it could be together.

I was wondering how extensive this was tested?

It seems there are no tests -- it'd be good to add some before this is merged:

it removed tests that were added to capture this earlier bug: Panic on too many/large transactions #20 -- even though this has been fixed, it doesn't hurt to keep the tests
may be good to add tests / properties capturing some of the desired properties of this new implementation (if the old implementation wasn't removed, but deprecated, there could have been some temporary tests, testing that given the same input, they produce the same output):

for any valid sequence of abci messages, consensus state should be valid
query/mempool messages should be processed irrespective of consensus connection
...

For the second property, I guess this could be simulated with some "dummy" abci app where some of the consensus connection response handling code (e.g. DeliverTx or begin block) are artificially expensive or non-terminating, so that they block and one can check that the query/mempool is still processed

tac0turtle · 2020-01-29T15:54:03Z

@tomtau @yihuang are there still any major concerns about this being too breaking?

0.33 is breaking anyway, so it could be together.

we could cut the 0.7 release with 0.33 tendermint then merge this PR and cut 0.8 if people would like to maintain the old functionality for a bit longer?

tomtau

0.33 is breaking anyway, so it could be together.

we could cut the 0.7 release with 0.33 tendermint then merge this PR and cut 0.8 if people would like to maintain the old functionality for a bit longer?

I think either option is fine -- can one get any downstream insights / statistics from dependabot, so that we can get some idea of the impact or ask for more opinions?
Besides tests, one other thing missing in this PR would be a very short "porting old abci applications" section / paragraph guide -- it seems it should be fairly straightforward, but still would be good to briefly document that (so one can get a quick idea of the required effort before jumping into it: new dependencies, separating out shared state, extra keywords, broken down application... currently, one would need to deduce that from reading the counter example)

README.md

devashishdxt · 2020-01-30T01:55:26Z

0.33 is breaking anyway, so it could be together.

we could cut the 0.7 release with 0.33 tendermint then merge this PR and cut 0.8 if people would like to maintain the old functionality for a bit longer?

I think either option is fine -- can one get any downstream insights / statistics from dependabot, so that we can get some idea of the impact or ask for more opinions?
Besides tests, one other thing missing in this PR would be a very short "porting old abci applications" section / paragraph guide -- it seems it should be fairly straightforward, but still would be good to briefly document that (so one can get a quick idea of the required effort before jumping into it: new dependencies, separating out shared state, extra keywords, broken down application... currently, one would need to deduce that from reading the counter example)

This PR is for merging abci-rs crate into this one. Extra documentation for upgrading existing code can be done in a different PR.

tac0turtle · 2020-02-07T10:17:02Z

How do people feel about merging this and having a follow-up PR for docs and anything else? We should just make issues with what we want in the follow-up PRs.

tomtau · 2020-02-08T08:08:40Z

How do people feel about merging this and having a follow-up PR for docs and anything else? We should just make issues with what we want in the follow-up PRs.

maybe ok to merge to develop, but not ok to release (merge to master) before at least a few tests are in place

src/server.rs

tomtau

TM v0.33 support was just released under 0.7 -- so there are some merge conflicts, so it'll be good to resolve those + bump to v0.8

devashishdxt · 2020-03-04T03:11:35Z

TM v0.33 support was just released under 0.7 -- so there are some merge conflicts, so it'll be good to resolve those + bump to v0.8

Resolved all the merge conflicts.

devashishdxt · 2020-03-04T03:19:32Z

Closing this PR. Will submit one without any merge issues in future.

tomtau

went through the changes in detail -- ok to merge to develop, but shouldn't be released / merged to master yet -- the biggest concern was put into an issue: #119

before merging it, it'll be good to rebase these changes on top of the latest develop, because at least Github UI diff shows some weird parts, so want to do a final double check what will actually end up there.

Other comments (can be addressed in a different PR):

sanity-check-validation of a consensus state machine is a kind of a random change / feature sneaked in here (not even mentioned in the changelog) that's orthogonal to the rest and will need more work -- e.g. since this is a dynamic check, instead of a static check (as originally envisioned with session types), it could also check other invariants (e.g. about block height, time, ...)
unix socket can be there as a default feature -- just distinguish those two connections based on the prefix in the connection string, similarly to Tendermint or KMS configurations
not sure why there are 100s of lines of code for these duplicate wrapper ABCI types that don't seem to do anything, but contain the same information as the protobuf types without some protobuf fluff? It'd be preferable to just use the protobuf types. If there's some strong reason for them, it'll be good to see whether the same can be achieved with some extra protobuf codegen configuration or if there can be a macro that will simplify this

tomtau · 2020-03-04T03:18:17Z

protobuf/github.com/tendermint/tendermint/abci/types/types.proto

+  int64 max_age_num_blocks = 1;
+  google.protobuf.Duration max_age_duration = 2 [(gogoproto.nullable)=false, (gogoproto.stdduration)=true];


I'm a bit puzzled why this diff shows up when it's like that on develop: https://github.com/tendermint/rust-abci/blob/develop/protobuf/abci/types/types.proto#L227

can you perhaps rebase on the latest develop?

tomtau · 2020-03-04T03:25:24Z

src/server.rs

+#[cfg(unix)]
+use std::path::PathBuf;


as the whole thing is breaking, perhaps this doesn't need to be feature guarded and TCP vs IPC can have a prefix, just like in Tendermint configurations?

tomtau · 2020-03-04T03:31:12Z

src/types/begin_block.rs

+use crate::proto::abci::{Event as ProtoEvent, RequestBeginBlock, ResponseBeginBlock};
+use crate::types::{Event, Evidence, Header, LastCommitInfo};
+
+#[derive(Debug, Default)]


Not sure what the point of these wrapper types is -- why not just use the protobuf types?

ruseinov · 2020-03-31T13:24:48Z

@devashishdxt Hello Devashish, I'm curious if there's a plan to revive this work anytime soon. The reason I'm asking is that I'm likely to go forward and implement an abci app in Rust. I used to be one of the developers https://github.com/iov-one/weave so looking into implementing something of sorts for Rust, a framework that would allow for custom routing/handling/middlewares.

devashishdxt · 2020-03-31T19:11:40Z

@devashishdxt Hello Devashish, I'm curious if there's a plan to revive this work anytime soon. The reason I'm asking is that I'm likely to go forward and implement an abci app in Rust. I used to be one of the developers https://github.com/iov-one/weave so looking into implementing something of sorts for Rust, a framework that would allow for custom routing/handling/middlewares.

Hi @ruseinov, I'm a bit busy with other things right now. I'll start working on this as soon as I have some free time. We still need to reach a consensus for some unresolved things (see comments above). In the meantime, you can try using abci-rs(Documentation) and suggest some improvements that you feel should be included in this crate. The final API design will not be much different from abci-rs. So, it'll be very straight forward to switch when updated version of this crate is available.

ruseinov · 2020-04-01T12:32:52Z

@devashishdxt that makes sense, I will look into this as soon as I start implementing the abci app.

tomtau requested review from ebuchman, tomtau, tac0turtle, melekes and liamsi January 10, 2020 05:38

tomtau suggested changes Jan 10, 2020

View reviewed changes

yihuang reviewed Jan 13, 2020

View reviewed changes

src/server.rs Outdated Show resolved Hide resolved

yihuang reviewed Jan 13, 2020

View reviewed changes

examples/counter.rs Show resolved Hide resolved

tomtau mentioned this pull request Jan 20, 2020

proto: update proto files & add third_party #116

Merged

6 tasks

erikgrinaker reviewed Jan 20, 2020

View reviewed changes

src/types.rs Show resolved Hide resolved

devashishdxt requested a review from erikgrinaker January 21, 2020 03:14

liamsi reviewed Jan 23, 2020

View reviewed changes

devashishdxt requested review from liamsi and tomtau January 24, 2020 02:46

tomtau suggested changes Jan 29, 2020

View reviewed changes

tomtau suggested changes Jan 30, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

tomtau reviewed Feb 28, 2020

View reviewed changes

src/server.rs Outdated Show resolved Hide resolved

devashishdxt requested a review from tomtau February 28, 2020 03:42

tomtau suggested changes Mar 4, 2020

View reviewed changes

devashishdxt and others added 3 commits March 4, 2020 10:27

Merge crate abci-rs into abci

62e989d

Merge branch 'develop' into abci-rs

72f8104

Get initialization status of ABCI Server from user

2c0f6ca

devashishdxt requested a review from tomtau March 4, 2020 03:11

devashishdxt closed this Mar 4, 2020

devashishdxt deleted the abci-rs branch March 4, 2020 03:32

tomtau reviewed Mar 4, 2020

View reviewed changes

tomtau mentioned this pull request Mar 4, 2020

rearchitected rust-abci has no tests #119

Open

tomtau mentioned this pull request Jun 30, 2020

High Level Goals for the next Third informalsystems/tendermint-rs#388

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge crate abci-rs into abci #112

Merge crate abci-rs into abci #112

devashishdxt commented Jan 10, 2020 •

edited

Loading

tomtau left a comment

devashishdxt commented Jan 10, 2020

tomtau commented Jan 10, 2020

tac0turtle commented Jan 10, 2020

tomtau commented Jan 11, 2020

devashishdxt commented Jan 20, 2020

tac0turtle commented Jan 22, 2020

devashishdxt commented Jan 22, 2020

liamsi left a comment

devashishdxt commented Jan 24, 2020

tomtau left a comment

tac0turtle commented Jan 29, 2020

tomtau left a comment

devashishdxt commented Jan 30, 2020

tac0turtle commented Feb 7, 2020

tomtau commented Feb 8, 2020 •

edited

Loading

tomtau left a comment

devashishdxt commented Mar 4, 2020

devashishdxt commented Mar 4, 2020

tomtau left a comment

tomtau Mar 4, 2020

tomtau Mar 4, 2020

tomtau Mar 4, 2020

ruseinov commented Mar 31, 2020

devashishdxt commented Mar 31, 2020

ruseinov commented Apr 1, 2020

		int64 max_age_num_blocks = 1;
		google.protobuf.Duration max_age_duration = 2 [(gogoproto.nullable)=false, (gogoproto.stdduration)=true];

Merge crate abci-rs into abci #112

Merge crate abci-rs into abci #112

Conversation

devashishdxt commented Jan 10, 2020 • edited Loading

tomtau left a comment

Choose a reason for hiding this comment

devashishdxt commented Jan 10, 2020

tomtau commented Jan 10, 2020

tac0turtle commented Jan 10, 2020

tomtau commented Jan 11, 2020

devashishdxt commented Jan 20, 2020

tac0turtle commented Jan 22, 2020

devashishdxt commented Jan 22, 2020

liamsi left a comment

Choose a reason for hiding this comment

devashishdxt commented Jan 24, 2020

tomtau left a comment

Choose a reason for hiding this comment

tac0turtle commented Jan 29, 2020

tomtau left a comment

Choose a reason for hiding this comment

devashishdxt commented Jan 30, 2020

tac0turtle commented Feb 7, 2020

tomtau commented Feb 8, 2020 • edited Loading

tomtau left a comment

Choose a reason for hiding this comment

devashishdxt commented Mar 4, 2020

devashishdxt commented Mar 4, 2020

tomtau left a comment

Choose a reason for hiding this comment

tomtau Mar 4, 2020

Choose a reason for hiding this comment

tomtau Mar 4, 2020

Choose a reason for hiding this comment

tomtau Mar 4, 2020

Choose a reason for hiding this comment

ruseinov commented Mar 31, 2020

devashishdxt commented Mar 31, 2020

ruseinov commented Apr 1, 2020

devashishdxt commented Jan 10, 2020 •

edited

Loading

tomtau commented Feb 8, 2020 •

edited

Loading