Skip to content

Tmsp proof#356

Closed
ethanfrey wants to merge 2 commits intounstablefrom
tmsp_proof
Closed

Tmsp proof#356
ethanfrey wants to merge 2 commits intounstablefrom
tmsp_proof

Conversation

@ethanfrey
Copy link
Contributor

Added rpc endpoints to expose new TMSP ProofSync endpoint. To address #355

This (all of these) PR was done in a sprint, there are probably some chunks of code needing cleanup, please give feedback. Also on the tmsp reorg.

@ethanfrey
Copy link
Contributor Author

Example usage (end-user version) is in the tests: https://github.com/tendermint/tendermint/blob/tmsp_proof/rpc/test/client_test.go#L174-L209

Is there somewhere else I should expose this?

}

func TMSPProofResult(key []byte, height int64) (ctypes.TMResult, error) {
fmt.Println("ONE")
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debug statement. removed and gone from git history

@codecov-io
Copy link

codecov-io commented Jan 10, 2017

Current coverage is 49.20% (diff: 0.00%)

Merging #356 into unstable will decrease coverage by 0.74%

@@           unstable       #356   diff @@
==========================================
  Files            36         36          
  Lines          4457       4447    -10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           2226       2188    -38   
- Misses         2018       2043    +25   
- Partials        213        216     +3   

Powered by Codecov. Last update 56341de...69aada4

@ebuchman
Copy link
Contributor

Why are we creating a new tmsp message? Why not just leave this to the Query?

@ethanfrey
Copy link
Contributor Author

Query just passes in an arbitrary byte struct meant to be deciphered by the client in any way they see fit, and returns what they see fit (often json in the examples).

Proof requires that the argument is a key and the result a merkle proof in hex-encoded binary form.

Sure, we could overload them, using the first byte of the argument to query to determine proof or query, but I don't see that makes the code better. It also adds more hidden requirements to the people implementing tmsp applications (meaning rather than getting a compile time error, suddenly queries would not work as expected).

We could also overload AppendTx and CheckTx by using the first byte as 0x0 or 0x1 to distinguish between them, but that wasn't done. I figured actions with different semantics should use different messages. Also, proof actually accepts two arguments, where query accepts one (the second being a currently unsupported blockheight, which is essential in Jae's opinion - and mine - to efficiently implementing IBC).

But then again, this was done quick to get something for Jae to demo. We can have a longer discussion on it and if you and Jae agree on another implementation, I'll accept your decision.

@ebuchman
Copy link
Contributor

Ok great. That makes sense. I think I'm convinced. Is there anything else you feel is missing from TMSP so far?

I'd rather try and get this merged in a non-breaking way as an optional feature for apps in 0.8.1. We can make it required in 0.9

@ethanfrey
Copy link
Contributor Author

ethanfrey commented Jan 12, 2017

Personally, I have a bit different idea of the architecture of tmsp, since that long thread with wolfspod a few months ago. I talked with Jae about it before implementing this, but he wanted something that would work now, without rethinking the design.

Fair enough, but when there is time to discuss that, I think it would be very important. You can accept or ignore the rest of this comment. I think it needs a bigger space to discuss than a github issue in any case.

< rant >
I think TMSP is the backbone for allowing the tendermint core to drive all changes on a TMSP Application. And for the Application to modify state in code (eg. validator set). I would actually remove all methods that do not change state or provide necessary information to change the state. Basically, everything that requires synchronization over all nodes should go through TMSP, nothing else.

In that respect, I would remove Query and Proof from tmsp.Application. I am not sure about Info, but if that is intended as the means for the tendermint core to do recovery with the app, then it makes sense in TMSP (tell me which blocks you committed before the crash... okay, now I will playback the rest of them for you).

I would also remove tmsp_query, tmsp_info, and tmsp_proof from the tendermin core rpc. (And probably the unsafe* methods as well)

Tendermint core is a consensus engine and handles all P2P between nodes, and provides quick BFT consensus on the global ordering of all action. That is amazingly powerful and it's mission should not grow beyond that.

A TMSP app can produce it's own web server, which handles all queries in whatever form it wishes without these restrictions from the tendermint rpc-tmsp communication layer. And without adding that load to the core. What if I want to provide a websocket listener when a key changes? How would I model that over tendermint rpc - tmsp and back? What if I want to configure my own CORS and allow gzip compression on large responses. Oh, and maybe throttle requests per IP....

All major languages have tooling to easily add these features to a web server. Let the app developers handle it. It is to many decisions and would take to much time to find the proper solution to everyone in tendermint core.

The main entry from a light client into any application built on tendermint should be some API chosen by the app developers. Independent of the tendermint API. This allows MUCH faster iteration and customization.

Tendermint core should just focus on being a consensus engine and generally hidden from the end user (like a cluster of etc nodes must communicate, possibly over geographically distributed areas, but are not exposed to the end users).

Rather than trying to do everything for the developers though the RPC, we could build some tools to make it easy to setup and then customize your API in a few languages and encourage the community to extend them. This would allow app devs from various languages to make the interfaces they need to work, without the barrier to entry of understanding and modifying tendermint core.
< /rant >

Hope that wasn't too much of an earful.....

@ebuchman
Copy link
Contributor

Strongly agree with the sentiment. Those rpc commands are useful for debugging.

Looking forward to all the upcoming changes we'll be rolling out together. I think we should maybe start a design doc for TMSP and work towards something we can hopefully think about finalizing in 0.9, as we move towards 1.0

@ethanfrey
Copy link
Contributor Author

To much renaming craziness. I have decided to kill this PR in favor of #372

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.

4 participants