From c48ff031cdee9cbe09a6f1441fc52d50ea8fb1cb Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 7 Jun 2018 16:01:07 -0700 Subject: [PATCH 1/8] add ADR-009 for ABCI design upgrade --- docs/architecture/adr-009-ABCI-design.md | 205 +++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 docs/architecture/adr-009-ABCI-design.md diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md new file mode 100644 index 00000000..d0e7e120 --- /dev/null +++ b/docs/architecture/adr-009-ABCI-design.md @@ -0,0 +1,205 @@ +# ADR 009: ABCI UX Improvements + +## Context + +The ABCI was first introduced in late 2015. It's purpose is to be: + +- a generic interface between state machines and their replication engines +- agnostic to the language the state machine is written in +- agnostic to the replication engine that drives it + +This means ABCI should provide an interface for both pluggable applications and +pluggable consensus engines. + +To achieve this, it uses Protocol Buffers for message types. The dominant +implementation is in Go. + +After some recent discussions with the community on github, the following were +identified as pain points: + +- Amino encoded types +- Managing validator sets +- Imports in the protobuf file + +See the following relevent github issues: +- TODO + +### Imports + +The native Protobuf library in Go generates code that is inellegant and difficult to work with. +The solution in the Go community is to use a fork of it called `gogoproto`. +While `gogoproto` is nice, it creates an additional dependency, and compiling +the protobuf types for other languages has been reported to fail when `gogoproto` is used. + +### Amino + +Amino is an encoding protocol designed to improve over insufficiencies of protobuf. +It's goal is to be Protobuf4. + +Many people are frustrated by incompatibility with protobuf, +and with the requirement for Amino to be used at all within ABCI. + +We intend to make Amino successful enough that we can eventually use it for ABCI +message types directly. By then it should be called Protobuf4. In the meantime, +we want it to be easy to use. + +### PubKey + +PubKeys were previously encoded using Amino (and before that, go-wire). + +### Addresses + +The address for an ED25519 pubkey is currently the RIPEMD160 of the Amino +encoded pubkey. + +### Validators + +To change the validator set, applications can return a list of validator updates +with ResponseEndBlock. In these updates, the public key *must* be included, +because Tendermint requires the public key to verify validator signatures. This +means ABCI developers have to work with PubKeys. That said, it would also be +convenient to work with address information, and for it to be simple to do so. + +### AbsentValidators + +Tendermint also provides a list of validators in BeginBlock who did not sign the +last block. This allows applications to reflect availability behaviour in the +application, for instance by punishing validators for not having votes included +in commits. + +### InitChain + +Tendermint passes in a list of validators here, and nothing else. It would +benefit the application to be able to control the initial validator set. For +instance the genesis file could include application-based information about the +initial validator set that the application could process to determine the +initial validator set. Additionally, InitChain would benefit from getting all +the genesis information. + + +## Decision + +### Imports + +Move away from gogoproto. In the short term, we will just maintain a second +protobuf file without the gogoproto annotations. In the medium term, we will +make copies of all the structs in Golang and shuttle back and forth. In the long +term, we will use Amino. + +### Amino + +To simplify ABCI application development in the short term, +Amino will be completely removed from the ABCI: + +- It will not be required for PubKey encoding +- It will not be required for computing PubKey addresses + +That said, we are working to make Amino a huge success, and to become Protobuf4. +To facilitate adoption and cross-language compatibility in the near-term, Amino +v1 will: + +- be fully compatible with the subset of Protobuf3 that excludes `oneof` +- use the Amino prefix system to provide interface types, as opposed to `oneof` + style union types. + +That said, an Amino v2 will be worked on to improve the performance of the +format and its useability in cryptographic applications. + + +### PubKey + +Encoding schemes infect software. As a generic middleware, ABCI aims to have +some cross scheme compatibility. For this it has no choice but to include opaque +bytes from time to time. While we will not enforce Amino encoding for these +bytes yet, we need to provide a type system. The simplest way to do this is to +use a type string. + +PubKey will now look like: + +``` +message PubKey { + string type + bytes data +} +``` + +where `type` can be: + +- "ed225519", with `data = ` +- "secp256k1", with `data = <33-byte OpenSSL compressed pubkey>` + +and generated types. At the least we should use a reflection-based protobuf so +we can just encode our own types, rather than using protobuf generated ones. + +### Addresses + +To simplify and improve computing addresses, we change it to the first 20-bytes of the SHA256 +of the raw 32-byte public key. + +We continue to use the Bitcoin address scheme for secp256k1 keys. + +### Validators + +Change the following: + +- Validator includes an optional `bytes address` field +- If the field is provided, it *MUST* correspond to the `pubkey.Address()` + + +``` +message Validator { + bytes address + PubKey pub_key + int64 power +} +``` + +### AbsentValidators + +To simplify this, RequestBeginBlock will include the complete validator set, +including the address, public key, and voting power of each validator, along +with a boolean for whether or not they voted: + +``` +message SigningValidator { + Validator validator + bool signed_last_block +} +``` + +### InitChain + +Change RequestInitChain and ResponseInitChain to + +``` +message RequestInitChain { + int64 time + string chain_id + ConsensusParams consensus_params + repeated Validator validators + bytes app_state_bytes +} + +message ResponseInitChain { + ConsensusParams consensus_params + repeated Validator validators +} +``` + +## Status + +Accepted. + +## Consequences + +### Positive + +- Easier for developers to build on the ABCI + +### Negative + +- Maintenance overhead of alternative type encoding scheme +- Performance overhead of passing all the validator info every block +- Maintenance overhead of duplicate types + +### Neutral From 7d82bdb3e6c0b8c42097e5b8afef92979c93e633 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 7 Jun 2018 16:58:52 -0700 Subject: [PATCH 2/8] adr-009: no pubkeys in beginblock --- docs/architecture/adr-009-ABCI-design.md | 47 ++++++++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index d0e7e120..2f9abba6 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -76,6 +76,10 @@ initial validator set that the application could process to determine the initial validator set. Additionally, InitChain would benefit from getting all the genesis information. +### Header + +ABCI provides the Header in RequestBeginBlock so the application can have +important information about the latest state of the blockchain. ## Decision @@ -140,11 +144,7 @@ We continue to use the Bitcoin address scheme for secp256k1 keys. ### Validators -Change the following: - -- Validator includes an optional `bytes address` field -- If the field is provided, it *MUST* correspond to the `pubkey.Address()` - +Add a `bytes address` field: ``` message Validator { @@ -154,19 +154,40 @@ message Validator { } ``` -### AbsentValidators +### RequestBeginBlock and AbsentValidators To simplify this, RequestBeginBlock will include the complete validator set, -including the address, public key, and voting power of each validator, along +including the address, and voting power of each validator, along with a boolean for whether or not they voted: ``` +message RequestBeginBlock { + bytes hash + Header header + repeated SigningValidator validators + repeated Evidence byzantine_validators +} + message SigningValidator { Validator validator bool signed_last_block } ``` +Note that in Validators in RequestBeginBlock, we DO NOT include public keys. Public keys are +larger than addresses and in the future, with quantum computers, will be much +larger. The overhead of passing them, especially during fast-sync, is +significant. + +Additional, addresses are changing to be simpler to compute, further removing +the need to include pubkeys here. + +In short, ABCI developers must be aware of both addresses and public keys. + +### ResponseEndBlock + +Since ResponseEndBlock includes Validator, it must now include their address. + ### InitChain Change RequestInitChain and ResponseInitChain to @@ -186,6 +207,12 @@ message ResponseInitChain { } ``` +### Header + +Now that Tendermint Amino will be compatible with Proto3, the Header in ABCI +should exactly match the Tendermint header - they will then be encoded +identically in ABCI and in Tendermint Core. + ## Status Accepted. @@ -195,11 +222,15 @@ Accepted. ### Positive - Easier for developers to build on the ABCI +- ABCI and Tendermint headers are identically serialized ### Negative - Maintenance overhead of alternative type encoding scheme -- Performance overhead of passing all the validator info every block +- Performance overhead of passing all validator info every block (at least its + only addresses, and not also pubkeys) - Maintenance overhead of duplicate types ### Neutral + +- ABCI developers must know about validator addresses From a25d18107407a3d1ac40112ced57252c8f3b48c8 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 7 Jun 2018 17:29:57 -0700 Subject: [PATCH 3/8] adr-009: add references --- docs/architecture/adr-009-ABCI-design.md | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index 2f9abba6..3c9cad81 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -1,5 +1,10 @@ # ADR 009: ABCI UX Improvements +## Changelog + +07-06-2018: Minor updates based on discussion with Jae +07-06-2018: Initial draft to match what was released in ABCI v0.11 + ## Context The ABCI was first introduced in late 2015. It's purpose is to be: @@ -21,8 +26,7 @@ identified as pain points: - Managing validator sets - Imports in the protobuf file -See the following relevent github issues: -- TODO +See the [references](#references) for more. ### Imports @@ -234,3 +238,18 @@ Accepted. ### Neutral - ABCI developers must know about validator addresses + +## References + +- [ABCI v0.10.3 Specification (before this + proposal)](https://github.com/tendermint/abci/blob/v0.10.3/specification.rst) +- [ABCI v0.11.0 Specification (implementing first draft of this + proposal)](https://github.com/tendermint/abci/blob/v0.11.0/specification.md) +- [Ed25519 addresses](https://github.com/tendermint/go-crypto/issues/103) +- [InitChain contains the + Genesis](https://github.com/tendermint/abci/issues/216) +- [PubKeys](https://github.com/tendermint/tendermint/issues/1524) +- [Notes on + Header](https://github.com/tendermint/tendermint/issues/1605) +- [Gogoproto issues](https://github.com/tendermint/abci/issues/256) +- [Absent Validators](https://github.com/tendermint/abci/issues/231) From 3e1684d2a2720ea254bdcb9db25b9ab0cea5048c Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 7 Jun 2018 17:33:01 -0700 Subject: [PATCH 4/8] adr-010-crypto-changes --- docs/architecture/adr-010-crypto-changes.md | 76 +++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 docs/architecture/adr-010-crypto-changes.md diff --git a/docs/architecture/adr-010-crypto-changes.md b/docs/architecture/adr-010-crypto-changes.md new file mode 100644 index 00000000..f61a2bd4 --- /dev/null +++ b/docs/architecture/adr-010-crypto-changes.md @@ -0,0 +1,76 @@ +# ADR 010: Crypto Changes + +## Context + +Tendermint is a cryptographic protocol that uses and composes a variety of cryptographic primitives. + +After nearly 4 years of development, Tendermint has recently undergone multiple security reviews to search for vulnerabilities and to assess the the use and composition of cryptographic primitives. + +### Hash Functions + +Tendermint currently uses RIPEMD160 universally as a hash function, most notably in its Merkle tree implementation. + +RIPEMD160 was chosen because it provides the shortest fingerprint that is long enough to be considered secure (ie. birthday bound of 80-bits). +It was also developed in the open academic community, unlike NSA-designed algorithms like SHA256. + +That said, the cryptographic community appears to unanimously agree on the security of SHA256. It has become a universal standard, especially now that SHA1 is broken, being required in TLS connections and having optimized support in hardware. + +### Merkle Trees + +Tendermint uses a simple Merkle tree to compute digests of large structures like transaction batches +and even blockchain headers. The Merkle tree length prefixes byte arrays before concatenating and hashing them. +It uses RIPEMD160. + +### Addresses + +ED25519 addresses are computed using the RIPEMD160 of the Amino encoding of the public key. + +### Authenticated Encryption + +Tendermint P2P connections use authenticated encryption to provide privacy and authentication in the communications. +This is done using the simple Station-to-Station protocol with the NaCL Ed25519 library. + +While there have been no vulnerabilities found in the implementation, there are some concerns: + +- NaCL uses Salsa20, a not-widely used and relatively out-dated stream cipher that has been obsoleted by ChaCha20 +- Connections use RIPEMD160 to compute a value that is used for the encryption nonce with subtle requirements on how it's used + +## Decision + +### Hash Functions + +Use the first 20-bytes of the SHA256 hash instead of RIPEMD160 for everything + +### Merkle Trees + +TODO + +### Addresses + +Compute ED25519 addresses as the first 20-bytes of the SHA256 of the raw 32-byte public key + +### Authenticated Encryption + +Make the following changes: + +- Use xChaCha20 instead of xSalsa20 - https://github.com/tendermint/tendermint/issues/1124 +- Use an HKDF instead of RIPEMD160 to compute nonces - https://github.com/tendermint/tendermint/issues/1165 + +## Status + +## Consequences + +### Positive + +- More modern and standard cryptographic functions with wider adoption and hardware acceleration + + +### Negative + +- Exact authenticated encryption construction isn't already provided in a well-used library + + +### Neutral + +## References + From 956e6d3435431b0c19e89aa93b6257df4fbafb0f Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 7 Jun 2018 18:39:29 -0700 Subject: [PATCH 5/8] change BeginBlock validators to LastCommitInfo --- docs/architecture/adr-009-ABCI-design.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index 3c9cad81..31a8dcfc 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -2,7 +2,7 @@ ## Changelog -07-06-2018: Minor updates based on discussion with Jae +07-06-2018: Some updates based on discussion with Jae 07-06-2018: Initial draft to match what was released in ABCI v0.11 ## Context @@ -168,10 +168,15 @@ with a boolean for whether or not they voted: message RequestBeginBlock { bytes hash Header header - repeated SigningValidator validators + LastCommitInfo last_commit_info repeated Evidence byzantine_validators } +message LastCommitInfo { + int32 CommitRound + repeated SigningValidator validators +} + message SigningValidator { Validator validator bool signed_last_block From f6ff6b0e1535418bc448692320b1931be6fbefb4 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 12 Jun 2018 13:53:40 +0400 Subject: [PATCH 6/8] use consistent naming for protobuf protobuf -> proto protobuf version X -> protoX --- docs/app-development.md | 50 ++++++++++++------------ docs/architecture/adr-009-ABCI-design.md | 10 ++--- docs/spec/blockchain/encoding.md | 6 +-- 3 files changed, 33 insertions(+), 33 deletions(-) diff --git a/docs/app-development.md b/docs/app-development.md index 0b6c402e..32abe215 100644 --- a/docs/app-development.md +++ b/docs/app-development.md @@ -99,14 +99,14 @@ performance, or otherwise enjoy programming, you may implement your own ABCI server using the Tendermint Socket Protocol, known affectionately as Teaspoon. The first step is still to auto-generate the relevant data types and codec in your language using `protoc`. Messages coming over -the socket are Protobuf3 encoded, but additionally length-prefixed to -facilitate use as a streaming protocol. Protobuf3 doesn't have an +the socket are proto3 encoded, but additionally length-prefixed to +facilitate use as a streaming protocol. proto3 doesn't have an official length-prefix standard, so we use our own. The first byte in the prefix represents the length of the Big Endian encoded length. The remaining bytes in the prefix are the Big Endian encoded length. -For example, if the Protobuf3 encoded ABCI message is 0xDEADBEEF (4 -bytes), the length-prefixed message is 0x0104DEADBEEF. If the Protobuf3 +For example, if the proto3 encoded ABCI message is 0xDEADBEEF (4 +bytes), the length-prefixed message is 0x0104DEADBEEF. If the proto3 encoded ABCI message is 65535 bytes long, the length-prefixed message would be like 0x02FFFF.... @@ -188,9 +188,9 @@ In Java: ResponseCheckTx requestCheckTx(RequestCheckTx req) { byte[] transaction = req.getTx().toByteArray(); - + // validate transaction - + if (notValid) { return ResponseCheckTx.newBuilder().setCode(CodeType.BadNonce).setLog("invalid tx").build(); } else { @@ -260,15 +260,15 @@ In Java: */ ResponseDeliverTx deliverTx(RequestDeliverTx request) { byte[] transaction = request.getTx().toByteArray(); - + // validate your transaction - + if (notValid) { return ResponseDeliverTx.newBuilder().setCode(CodeType.BadNonce).setLog("transaction was invalid").build(); } else { ResponseDeliverTx.newBuilder().setCode(CodeType.OK).build(); } - + } ### Commit @@ -302,10 +302,10 @@ In go: In Java: ResponseCommit requestCommit(RequestCommit requestCommit) { - + // update the internal app-state byte[] newAppState = calculateAppState(); - + // and return it to the node return ResponseCommit.newBuilder().setCode(CodeType.OK).setData(ByteString.copyFrom(newAppState)).build(); } @@ -326,7 +326,7 @@ In go: func (app *PersistentKVStoreApplication) BeginBlock(params types.RequestBeginBlock) { // update latest block info app.blockHeader = params.Header - + // reset valset changes app.changes = make([]*types.Validator, 0) } @@ -337,14 +337,14 @@ In Java: * all types come from protobuf definition */ ResponseBeginBlock requestBeginBlock(RequestBeginBlock req) { - + Header header = req.getHeader(); byte[] prevAppHash = header.getAppHash().toByteArray(); long prevHeight = header.getHeight(); long numTxs = header.getNumTxs(); - + // run your pre-block logic. Maybe prepare a state snapshot, message components, etc - + return ResponseBeginBlock.newBuilder().build(); } @@ -377,10 +377,10 @@ In Java: ResponseEndBlock requestEndBlock(RequestEndBlock req) { final long currentHeight = req.getHeight(); final byte[] validatorPubKey = getValPubKey(); - + ResponseEndBlock.Builder builder = ResponseEndBlock.newBuilder(); builder.addDiffs(1, Types.Validator.newBuilder().setPower(10L).setPubKey(ByteString.copyFrom(validatorPubKey)).build()); - + return builder.build(); } @@ -437,25 +437,25 @@ In Java: ResponseQuery requestQuery(RequestQuery req) { final boolean isProveQuery = req.getProve(); final ResponseQuery.Builder responseBuilder = ResponseQuery.newBuilder(); - + if (isProveQuery) { com.app.example.ProofResult proofResult = generateProof(req.getData().toByteArray()); final byte[] proofAsByteArray = proofResult.getAsByteArray(); - + responseBuilder.setProof(ByteString.copyFrom(proofAsByteArray)); responseBuilder.setKey(req.getData()); responseBuilder.setValue(ByteString.copyFrom(proofResult.getData())); responseBuilder.setLog(result.getLogValue()); } else { byte[] queryData = req.getData().toByteArray(); - + final com.app.example.QueryResult result = generateQueryResult(queryData); - + responseBuilder.setIndex(result.getIndex()); responseBuilder.setValue(ByteString.copyFrom(result.getValue())); responseBuilder.setLog(result.getLogValue()); } - + return responseBuilder.build(); } @@ -515,13 +515,13 @@ In Java: ResponseInitChain requestInitChain(RequestInitChain req) { final int validatorsCount = req.getValidatorsCount(); final List validatorsList = req.getValidatorsList(); - + validatorsList.forEach((validator) -> { long power = validator.getPower(); byte[] validatorPubKey = validator.getPubKey().toByteArray(); - + // do somehing for validator setup in app }); - + return ResponseInitChain.newBuilder().build(); } diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index 31a8dcfc..5de37f9e 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -30,7 +30,7 @@ See the [references](#references) for more. ### Imports -The native Protobuf library in Go generates code that is inellegant and difficult to work with. +The native proto library in Go generates code that is inellegant and difficult to work with. The solution in the Go community is to use a fork of it called `gogoproto`. While `gogoproto` is nice, it creates an additional dependency, and compiling the protobuf types for other languages has been reported to fail when `gogoproto` is used. @@ -38,13 +38,13 @@ the protobuf types for other languages has been reported to fail when `gogoproto ### Amino Amino is an encoding protocol designed to improve over insufficiencies of protobuf. -It's goal is to be Protobuf4. +It's goal is to be proto4. Many people are frustrated by incompatibility with protobuf, and with the requirement for Amino to be used at all within ABCI. We intend to make Amino successful enough that we can eventually use it for ABCI -message types directly. By then it should be called Protobuf4. In the meantime, +message types directly. By then it should be called proto4. In the meantime, we want it to be easy to use. ### PubKey @@ -102,11 +102,11 @@ Amino will be completely removed from the ABCI: - It will not be required for PubKey encoding - It will not be required for computing PubKey addresses -That said, we are working to make Amino a huge success, and to become Protobuf4. +That said, we are working to make Amino a huge success, and to become proto4. To facilitate adoption and cross-language compatibility in the near-term, Amino v1 will: -- be fully compatible with the subset of Protobuf3 that excludes `oneof` +- be fully compatible with the subset of proto3 that excludes `oneof` - use the Amino prefix system to provide interface types, as opposed to `oneof` style union types. diff --git a/docs/spec/blockchain/encoding.md b/docs/spec/blockchain/encoding.md index e59d3c8c..1c33aa1f 100644 --- a/docs/spec/blockchain/encoding.md +++ b/docs/spec/blockchain/encoding.md @@ -2,8 +2,8 @@ ## Amino -Tendermint uses the Protobuf3 derivative [Amino](https://github.com/tendermint/go-amino) for all data structures. -Think of Amino as an object-oriented Protobuf3 with native JSON support. +Tendermint uses the proto3 derivative [Amino](https://github.com/tendermint/go-amino) for all data structures. +Think of Amino as an object-oriented proto3 with native JSON support. The goal of the Amino encoding protocol is to bring parity between application logic objects and persistence objects. @@ -21,7 +21,7 @@ arbitrary object and return the Amino encoded bytes. ## Byte Arrays The encoding of a byte array is simply the raw-bytes prefixed with the length of -the array as a `UVarint` (what Protobuf calls a `Varint`). +the array as a `UVarint` (what proto calls a `Varint`). For details on varints, see the [protobuf spec](https://developers.google.com/protocol-buffers/docs/encoding#varints). From 500fca8efe373f09eaab0b035ddf3bd532fa730c Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 23 Jun 2018 09:20:08 -0400 Subject: [PATCH 7/8] fixes from review --- docs/architecture/adr-009-ABCI-design.md | 32 ++++++++++++++------- docs/architecture/adr-010-crypto-changes.md | 4 ++- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index 5de37f9e..171204c1 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -16,7 +16,7 @@ The ABCI was first introduced in late 2015. It's purpose is to be: This means ABCI should provide an interface for both pluggable applications and pluggable consensus engines. -To achieve this, it uses Protocol Buffers for message types. The dominant +To achieve this, it uses Protocol Buffers (proto3) for message types. The dominant implementation is in Go. After some recent discussions with the community on github, the following were @@ -30,8 +30,10 @@ See the [references](#references) for more. ### Imports -The native proto library in Go generates code that is inellegant and difficult to work with. -The solution in the Go community is to use a fork of it called `gogoproto`. +The native proto library in Go generates inflexible and verbose code. +Many in the Go community have adopted a fork called +[gogoproto](https://github.com/gogo/protobuf) that provides a +variety of features aimed to improve the developer experience. While `gogoproto` is nice, it creates an additional dependency, and compiling the protobuf types for other languages has been reported to fail when `gogoproto` is used. @@ -49,12 +51,16 @@ we want it to be easy to use. ### PubKey -PubKeys were previously encoded using Amino (and before that, go-wire). +PubKeys are encoded using Amino (and before that, go-wire). +Ideally, PubKeys are an interface type where we don't know all the +implementation types, so its unfitting to use `oneof` or `enum`. ### Addresses -The address for an ED25519 pubkey is currently the RIPEMD160 of the Amino -encoded pubkey. +The address for ED25519 pubkey is the RIPEMD160 of the Amino +encoded pubkey. This introduces an Amino dependency in the address generation, +a functionality that is widely required and should be easy to compute as +possible. ### Validators @@ -136,8 +142,9 @@ where `type` can be: - "ed225519", with `data = ` - "secp256k1", with `data = <33-byte OpenSSL compressed pubkey>` -and generated types. At the least we should use a reflection-based protobuf so -we can just encode our own types, rather than using protobuf generated ones. + +As we want to retain flexibility here, and since ideally, PubKey would be an +interface type, we do not use `enum` or `oneof`. ### Addresses @@ -199,7 +206,7 @@ Since ResponseEndBlock includes Validator, it must now include their address. ### InitChain -Change RequestInitChain and ResponseInitChain to +Change RequestInitChain to give the app all the information from the genesis file: ``` message RequestInitChain { @@ -209,7 +216,12 @@ message RequestInitChain { repeated Validator validators bytes app_state_bytes } +``` +Change ResponseInitChain to allow the app to specify the initial validator set +and consensus parameters. + +``` message ResponseInitChain { ConsensusParams consensus_params repeated Validator validators @@ -218,7 +230,7 @@ message ResponseInitChain { ### Header -Now that Tendermint Amino will be compatible with Proto3, the Header in ABCI +Now that Tendermint Amino will be compatible with proto3, the Header in ABCI should exactly match the Tendermint header - they will then be encoded identically in ABCI and in Tendermint Core. diff --git a/docs/architecture/adr-010-crypto-changes.md b/docs/architecture/adr-010-crypto-changes.md index f61a2bd4..cfe61842 100644 --- a/docs/architecture/adr-010-crypto-changes.md +++ b/docs/architecture/adr-010-crypto-changes.md @@ -8,7 +8,7 @@ After nearly 4 years of development, Tendermint has recently undergone multiple ### Hash Functions -Tendermint currently uses RIPEMD160 universally as a hash function, most notably in its Merkle tree implementation. +Tendermint uses RIPEMD160 universally as a hash function, most notably in its Merkle tree implementation. RIPEMD160 was chosen because it provides the shortest fingerprint that is long enough to be considered secure (ie. birthday bound of 80-bits). It was also developed in the open academic community, unlike NSA-designed algorithms like SHA256. @@ -24,6 +24,8 @@ It uses RIPEMD160. ### Addresses ED25519 addresses are computed using the RIPEMD160 of the Amino encoding of the public key. +RIPEMD160 is generally considered an outdated hash function, and is much slower +than more modern functions like SHA256 or Blake2. ### Authenticated Encryption From 121508195185ba84ad16d9b3fdb43333234ef918 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 23 Jun 2018 09:28:12 -0400 Subject: [PATCH 8/8] adr: update readme --- docs/architecture/README.md | 23 ++++++++++++++++++++--- docs/architecture/adr-009-ABCI-design.md | 1 + 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 9e41d306..1cfc7ddc 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -1,5 +1,22 @@ -# Architecture Decision Records +# Architecture Decision Records (ADR) -This is a location to record all high-level architecture decisions in the tendermint project. Not the implementation details, but the reasoning that happened. This should be refered to for guidance of the "right way" to extend the application. And if we notice that the original decisions were lacking, we should have another open discussion, record the new decisions here, and then modify the code to match. +This is a location to record all high-level architecture decisions in the tendermint project. -Read up on the concept in this [blog post](https://product.reverb.com/documenting-architecture-decisions-the-reverb-way-a3563bb24bd0#.78xhdix6t). +You can read more about the ADR concept in this [blog post](https://product.reverb.com/documenting-architecture-decisions-the-reverb-way-a3563bb24bd0#.78xhdix6t). + +An ADR should provide: + +- Context on the relevant goals and the current state +- Proposed changes to achieve the goals +- Summary of pros and cons +- References +- Changelog + +Note the distinction between an ADR and a spec. The ADR provides the context, intuition, reasoning, and +justification for a change in architecture, or for the architecture of something +new. The spec is much more compressed and streamlined summary of everything as +it stands today. + +If recorded decisions turned out to be lacking, convene a discussion, record the new decisions here, and then modify the code to match. + +Note the context/background should be written in the present tense. diff --git a/docs/architecture/adr-009-ABCI-design.md b/docs/architecture/adr-009-ABCI-design.md index 171204c1..8b85679b 100644 --- a/docs/architecture/adr-009-ABCI-design.md +++ b/docs/architecture/adr-009-ABCI-design.md @@ -2,6 +2,7 @@ ## Changelog +23-06-2018: Some minor fixes from review 07-06-2018: Some updates based on discussion with Jae 07-06-2018: Initial draft to match what was released in ABCI v0.11