From 5955eddc7d6e344e8f18ad6823e3bd8ac3fb84b7 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Fri, 27 Jul 2018 13:18:21 -0700 Subject: [PATCH 01/19] ADR: Fix malleability problems in Secp256k1 signatures Previously you could not assume that your transaction hash would appear on chain. --- .../architecture/adr-014-secp-malleability.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 docs/architecture/adr-014-secp-malleability.md diff --git a/docs/architecture/adr-014-secp-malleability.md b/docs/architecture/adr-014-secp-malleability.md new file mode 100644 index 000000000..ee196f489 --- /dev/null +++ b/docs/architecture/adr-014-secp-malleability.md @@ -0,0 +1,53 @@ +# ADR 014: Secp256k1 Signature Malleability + +## Context + +Secp256k1 has two layers of malleability. +The signer has a random nonce, and thus can produce many different valid signatures. +This ADR is not concerned with that. +The second layer of malleability basically allows one who is given a signature +to produce exactly one more valid signature for the same message from the same public key. +(They don't even have to know the message!) +The math behind this will be explained in the subsequent section. + +Note that in many downstream applications, signatures will appear in a transaction, and therefore in the tx hash. +This means that if someone broadcasts a transaction with secp256k1 signature, the signature can be altered into the other form by anyone in the p2p network. +Thus the tx hash will change, and this altered tx hash may be committed instead. +This breaks the assumption that you can broadcast a valid transaction and just wait for its hash to be included on chain. +You may not even know to increment your sequence number for example. +Removing this second layer of signature malleability concerns could ease downstream development. + +### ECDSA context + +Secp256k1 is ECDSA over a particular curve. +The signature is of the form `(r, s)`, where `s` is an elliptic curve group element. +However `(r, -s)` is also another valid solution. +Note that anyone can negate a group element, and therefore can get this second signature. + +## Decision + +We can just distinguish a canonical form for the ECDSA signatures. +Then we require that all ECDSA signatures be in the canonical form between the two. + +The canonical form is rather easy to define and check. +It would just be the smaller of the two y coordinates for the given x coordinate, defined lexicographically. +Example of other systems using this: https://github.com/zkcrypto/pairing/tree/master/src/bls12_381#serialization. + +## Proposed Implementation + +Fork https://github.com/btcsuite/btcd, and just update the [parse sig method](https://github.com/btcsuite/btcd/blob/master/btcec/signature.go#195) and serialize functions to enforce our canonical form. + +## Status + +Proposed. + +## Consequences + +### Positive +* Lets us maintain the ability to expect a tx hash to appear in the blockchain. + +### Negative +* More work in all future implementations (Though this is a very simple check) +* Requires us to maintain another fork + +### Neutral From a2debe57c7b310520318df72c81bfe8c0816a4cb Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Fri, 27 Jul 2018 18:09:23 -0700 Subject: [PATCH 02/19] [ADR] Proposal for encoding symmetric cryptography --- docs/architecture/adr-015-symmetric-crypto.md | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 docs/architecture/adr-015-symmetric-crypto.md diff --git a/docs/architecture/adr-015-symmetric-crypto.md b/docs/architecture/adr-015-symmetric-crypto.md new file mode 100644 index 000000000..e798f8492 --- /dev/null +++ b/docs/architecture/adr-015-symmetric-crypto.md @@ -0,0 +1,79 @@ +# ADR 015: Need for symmetric cryptography + +## Context + +We require symmetric ciphers to handle how we encrypt keys in the sdk, +and to potentially encrypt `priv_validator.json` in tendermint. + +Currently we use AEAD's to support symmetric encryption, +which is great since we want data integrity in addition to privacy and authenticity. +We don't currently have a scenario where we want to encrypt without data integrity, +so it is fine to optimize our code to just use AEAD's. +Currently there is not a way to switch out AEAD's easily, this ADR outlines a way +to easily swap these out. + +### How do we encrypt with AEAD's + +AEAD's typically require a nonce in addition to the key. +For the purposes we require symmetric cryptography for, +we need encryption to be stateless. +Because of this we use random nonces. +(Thus the AEAD must support random nonces) + +We currently construct a random nonce, and encrypt the data with it. +The returned value is `nonce || encrypted data`. +The limitation of this is that does not provide a way to identify +which algorithm was used in encryption. +Consequently decryption with multiple algoritms is sub-optimal. +(You have to try them all) + +## Decision + +We should create the following two methods in a new `crypto/encoding/symmetric` package: +```golang +func EncryptSymmetric(aead cipher.AEAD, plaintext []byte) (ciphertext []byte, err error) +func DecryptSymmetric(key []byte, ciphertext []byte) (plaintext []byte, err error) +func RegisterSymmetric(aead cipher.AEAD, algo_name string, NewAead func(key []byte) (cipher.Aead, error)) error +``` + +This allows you to specify the algorithm in encryption, but not have to specify +it in decryption. +This is intended for ease of use in downstream applications, in addition to people +looking at the file directly. +One downside is that for the encrypt function you must have already initialized an AEAD, +but I don't really see this as an issue. + +If there is no error in encryption, EncryptSymmetric will return `algo_name || nonce || aead_ciphertext`. +This requires a mapping from aead type to name. +We can achieve this via reflection. +```golang +func getType(myvar interface{}) string { + if t := reflect.TypeOf(myvar); t.Kind() == reflect.Ptr { + return "*" + t.Elem().Name() + } else { + return t.Name() + } +} +``` +Then we maintain a map from the name returned from `getType(aead)` to `algo_name`. + +In decryption, we read the `algo_name`, and then instantiate a new AEAD with the key. +Then we call the AEAD's decrypt method on the provided nonce/ciphertext. + +`RegisterSymmetric` allows a downstream user to add their own desired AEAD to the symmetric package. + +## Status + +Proposed. + +## Consequences + +### Positive +* Allows us to support new AEAD's, in a way that makes decryption easier +* Allows downstream users to add their own AEAD + +### Negative + +### Neutral +* Caller has to instantiate the AEAD with the private key. +However it forces them to be aware of what signing algorithm they are using, which is a positive. \ No newline at end of file From af2894c0f80176759e9ac0631696a9d7799cc22d Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Fri, 27 Jul 2018 19:27:25 -0700 Subject: [PATCH 03/19] (squash this) improve grammar. --- docs/architecture/adr-014-secp-malleability.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-014-secp-malleability.md b/docs/architecture/adr-014-secp-malleability.md index ee196f489..b84a47852 100644 --- a/docs/architecture/adr-014-secp-malleability.md +++ b/docs/architecture/adr-014-secp-malleability.md @@ -14,7 +14,9 @@ Note that in many downstream applications, signatures will appear in a transacti This means that if someone broadcasts a transaction with secp256k1 signature, the signature can be altered into the other form by anyone in the p2p network. Thus the tx hash will change, and this altered tx hash may be committed instead. This breaks the assumption that you can broadcast a valid transaction and just wait for its hash to be included on chain. -You may not even know to increment your sequence number for example. +One example is if you are broadcasting a tx in cosmos, +and you wait for it to appear on chain before incrementing your sequence number. +You may never increment your sequence number if a different tx hash got committed. Removing this second layer of signature malleability concerns could ease downstream development. ### ECDSA context @@ -27,9 +29,9 @@ Note that anyone can negate a group element, and therefore can get this second s ## Decision We can just distinguish a canonical form for the ECDSA signatures. -Then we require that all ECDSA signatures be in the canonical form between the two. +Then we require that all ECDSA signatures be in the form which we defined as canonical. -The canonical form is rather easy to define and check. +A canonical form is rather easy to define and check. It would just be the smaller of the two y coordinates for the given x coordinate, defined lexicographically. Example of other systems using this: https://github.com/zkcrypto/pairing/tree/master/src/bls12_381#serialization. From caef5dcd6953f47f661dbbea6dc5102530d5a643 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sat, 28 Jul 2018 04:14:07 -0700 Subject: [PATCH 04/19] (Squash this) forgot to say that algo_name should be length prefixed --- docs/architecture/adr-015-symmetric-crypto.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr-015-symmetric-crypto.md b/docs/architecture/adr-015-symmetric-crypto.md index e798f8492..166d99c0f 100644 --- a/docs/architecture/adr-015-symmetric-crypto.md +++ b/docs/architecture/adr-015-symmetric-crypto.md @@ -44,7 +44,10 @@ One downside is that for the encrypt function you must have already initialized but I don't really see this as an issue. If there is no error in encryption, EncryptSymmetric will return `algo_name || nonce || aead_ciphertext`. -This requires a mapping from aead type to name. +`algo_name` should be length prefixed, using standard varuint encoding. +This will be binary data, but thats not a problem considering the nonce and ciphertext are also binary. + +This solution requires a mapping from aead type to name. We can achieve this via reflection. ```golang func getType(myvar interface{}) string { From c03ad56d554a929a59a48dd28667ccefc479bf5c Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sat, 28 Jul 2018 04:23:22 -0700 Subject: [PATCH 05/19] (squash this) Note that this breaks existing keys. --- docs/architecture/adr-015-symmetric-crypto.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/architecture/adr-015-symmetric-crypto.md b/docs/architecture/adr-015-symmetric-crypto.md index 166d99c0f..fbaee7a1d 100644 --- a/docs/architecture/adr-015-symmetric-crypto.md +++ b/docs/architecture/adr-015-symmetric-crypto.md @@ -65,6 +65,13 @@ Then we call the AEAD's decrypt method on the provided nonce/ciphertext. `RegisterSymmetric` allows a downstream user to add their own desired AEAD to the symmetric package. +## Implementation strategy + +The golang implementation of what is proposed is rather straight forward. +The concern is that we will break existing private keys if we just switch to this. +If this is concerning, we can make a simple script which doesn't require decoding privkeys, +for converting from the old format to the new one. + ## Status Proposed. @@ -76,6 +83,8 @@ Proposed. * Allows downstream users to add their own AEAD ### Negative +* We will have to break all private keys stored on disk. +They can be recovered using seed words, and upgrade scripts are simple. ### Neutral * Caller has to instantiate the AEAD with the private key. From ce9ddc7cd7204a6d966dd20c7a5deef8a3161ebb Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sat, 28 Jul 2018 06:32:54 -0700 Subject: [PATCH 06/19] (squash this) Note not to overwrite aead's. --- docs/architecture/adr-015-symmetric-crypto.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/architecture/adr-015-symmetric-crypto.md b/docs/architecture/adr-015-symmetric-crypto.md index fbaee7a1d..7a587d18a 100644 --- a/docs/architecture/adr-015-symmetric-crypto.md +++ b/docs/architecture/adr-015-symmetric-crypto.md @@ -64,6 +64,8 @@ In decryption, we read the `algo_name`, and then instantiate a new AEAD with the Then we call the AEAD's decrypt method on the provided nonce/ciphertext. `RegisterSymmetric` allows a downstream user to add their own desired AEAD to the symmetric package. +It will error if the AEAD name is already registered. +This prevents a malicious import from modifying / nullifying an AEAD at runtime. ## Implementation strategy From 3d5d254932bd53808f64410b3dcc067ede4840f2 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Sat, 28 Jul 2018 20:40:21 -0700 Subject: [PATCH 07/19] (squash this) Mixed up field element and curve element. Idea still stands. --- docs/architecture/adr-014-secp-malleability.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-014-secp-malleability.md b/docs/architecture/adr-014-secp-malleability.md index b84a47852..f0db5743d 100644 --- a/docs/architecture/adr-014-secp-malleability.md +++ b/docs/architecture/adr-014-secp-malleability.md @@ -22,7 +22,8 @@ Removing this second layer of signature malleability concerns could ease downstr ### ECDSA context Secp256k1 is ECDSA over a particular curve. -The signature is of the form `(r, s)`, where `s` is an elliptic curve group element. +The signature is of the form `(r, s)`, where `s` is a field element. +(The particular field is the `Z_n`, where the elliptic curve has order `n`) However `(r, -s)` is also another valid solution. Note that anyone can negate a group element, and therefore can get this second signature. @@ -30,10 +31,13 @@ Note that anyone can negate a group element, and therefore can get this second s We can just distinguish a canonical form for the ECDSA signatures. Then we require that all ECDSA signatures be in the form which we defined as canonical. +We reject signatures in non-canonical form. A canonical form is rather easy to define and check. -It would just be the smaller of the two y coordinates for the given x coordinate, defined lexicographically. -Example of other systems using this: https://github.com/zkcrypto/pairing/tree/master/src/bls12_381#serialization. +It would just be the smaller of the two values for `s`, defined lexicographically. +This is a simple check, instead of checking if `s < n`, instead check `s <= (n - 1)/2`. +An example of another cryptosystem using this +is the parity definition here https://github.com/zkcrypto/pairing/pull/30#issuecomment-372910663. ## Proposed Implementation From b33f73eaf13258147c191c416f9b7a11226ee460 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 2 Aug 2018 16:33:34 +0400 Subject: [PATCH 08/19] stop autofile and autogroup properly NOTE: from the ticker#Stop documentation: ``` Stop does not close the channel, to prevent a read from the channel succeeding incorrectly. https://golang.org/src/time/tick.go?s=1318:1341#L35 ``` Refs #2072 --- libs/autofile/autofile.go | 28 +++++++++++++++------------- libs/autofile/group.go | 14 ++++---------- libs/autofile/group_test.go | 2 +- 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/libs/autofile/autofile.go b/libs/autofile/autofile.go index 313da6789..2f1bb4fd5 100644 --- a/libs/autofile/autofile.go +++ b/libs/autofile/autofile.go @@ -35,18 +35,20 @@ const autoFileOpenDuration = 1000 * time.Millisecond // Automatically closes and re-opens file for writing. // This is useful for using a log file with the logrotate tool. type AutoFile struct { - ID string - Path string - ticker *time.Ticker - mtx sync.Mutex - file *os.File + ID string + Path string + ticker *time.Ticker + tickerStopped chan struct{} // closed when ticker is stopped + mtx sync.Mutex + file *os.File } func OpenAutoFile(path string) (af *AutoFile, err error) { af = &AutoFile{ - ID: cmn.RandStr(12) + ":" + path, - Path: path, - ticker: time.NewTicker(autoFileOpenDuration), + ID: cmn.RandStr(12) + ":" + path, + Path: path, + ticker: time.NewTicker(autoFileOpenDuration), + tickerStopped: make(chan struct{}), } if err = af.openFile(); err != nil { return @@ -58,18 +60,18 @@ func OpenAutoFile(path string) (af *AutoFile, err error) { func (af *AutoFile) Close() error { af.ticker.Stop() + close(af.tickerStopped) err := af.closeFile() sighupWatchers.removeAutoFile(af) return err } func (af *AutoFile) processTicks() { - for { - _, ok := <-af.ticker.C - if !ok { - return // Done. - } + select { + case <-af.ticker.C: af.closeFile() + case <-af.tickerStopped: + return } } diff --git a/libs/autofile/group.go b/libs/autofile/group.go index b4368ed9e..9b78c5110 100644 --- a/libs/autofile/group.go +++ b/libs/autofile/group.go @@ -199,21 +199,15 @@ func (g *Group) Flush() error { } func (g *Group) processTicks() { - for { - _, ok := <-g.ticker.C - if !ok { - return // Done. - } + select { + case <-g.ticker.C: g.checkHeadSizeLimit() g.checkTotalSizeLimit() + case <-g.Quit(): + return } } -// NOTE: for testing -func (g *Group) stopTicker() { - g.ticker.Stop() -} - // NOTE: this function is called manually in tests. func (g *Group) checkHeadSizeLimit() { limit := g.HeadSizeLimit() diff --git a/libs/autofile/group_test.go b/libs/autofile/group_test.go index c7e8725cf..d6b10a420 100644 --- a/libs/autofile/group_test.go +++ b/libs/autofile/group_test.go @@ -26,7 +26,7 @@ func createTestGroup(t *testing.T, headSizeLimit int64) *Group { g, err := OpenGroup(headPath) require.NoError(t, err, "Error opening Group") g.SetHeadSizeLimit(headSizeLimit) - g.stopTicker() + g.ticker.Stop() require.NotEqual(t, nil, g, "Failed to create Group") return g } From 4c5a143a70c804126fe169e44ebf2bdd95fd8a5f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 2 Aug 2018 16:36:28 +0400 Subject: [PATCH 09/19] respawn receiveRoutine so we can properly exit Closes #2072 --- consensus/state.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/state.go b/consensus/state.go index f66a872eb..435427a91 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -556,6 +556,7 @@ func (cs *ConsensusState) receiveRoutine(maxSteps int) { defer func() { if r := recover(); r != nil { cs.Logger.Error("CONSENSUS FAILURE!!!", "err", r, "stack", string(debug.Stack())) + go cs.receiveRoutine(0) } }() @@ -588,7 +589,6 @@ func (cs *ConsensusState) receiveRoutine(maxSteps int) { // go to the next step cs.handleTimeout(ti, rs) case <-cs.Quit(): - // NOTE: the internalMsgQueue may have signed messages from our // priv_val that haven't hit the WAL, but its ok because // priv_val tracks LastSig From 8ed99c2c136d4833376be3cc41bc673729cf722a Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 2 Aug 2018 16:42:25 +0400 Subject: [PATCH 10/19] exit from initSighupWatcher child goroutine also, remove excessive log message Refs #2072 --- libs/autofile/sighup_watcher.go | 12 +++++++++--- p2p/pex/addrbook.go | 1 - 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/libs/autofile/sighup_watcher.go b/libs/autofile/sighup_watcher.go index 56fbd4d86..f72f12fcd 100644 --- a/libs/autofile/sighup_watcher.go +++ b/libs/autofile/sighup_watcher.go @@ -18,13 +18,19 @@ var sighupCounter int32 // For testing func initSighupWatcher() { sighupWatchers = newSighupWatcher() - c := make(chan os.Signal, 1) - signal.Notify(c, syscall.SIGHUP) + hup := make(chan os.Signal, 1) + signal.Notify(hup, syscall.SIGHUP) + + quit := make(chan os.Signal, 1) + signal.Notify(quit, os.Interrupt, syscall.SIGTERM) go func() { - for range c { + select { + case <-hup: sighupWatchers.closeAll() atomic.AddInt32(&sighupCounter, 1) + case <-quit: + return } }() } diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index ef7d7edaa..9596b1d76 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -496,7 +496,6 @@ out: } saveFileTicker.Stop() a.saveToFile(a.filePath) - a.Logger.Info("Address handler done") } //---------------------------------------------------------- From b82138b0025d19cdb155c003ce7a8bd019bdfdc5 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 2 Aug 2018 16:48:12 +0400 Subject: [PATCH 11/19] update changelog --- CHANGELOG_PENDING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index e85a6ed64..010aea40e 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -26,3 +26,4 @@ BUG FIXES: - [common] Safely handle cases where atomic write files already exist [#2109](https://github.com/tendermint/tendermint/issues/2109) - [privval] fix a deadline for accepting new connections in socket private validator. +- [node] Fully exit when CTRL-C is pressed even if consensus state panics [#2072] From d579f4c6103c99b78902b8bdf55315d2b9c315ae Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 2 Aug 2018 17:54:55 +0400 Subject: [PATCH 12/19] update genproto Closes #1944 --- Gopkg.lock | 15 ++++++++------- Gopkg.toml | 14 -------------- 2 files changed, 8 insertions(+), 21 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index aa9a1ff2f..557e2b181 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -15,7 +15,7 @@ name = "github.com/btcsuite/btcd" packages = ["btcec"] pruneopts = "UT" - revision = "9a2f9524024889e129a5422aca2cff73cb3eabf6" + revision = "f5e261fc9ec3437697fb31d8b38453c293204b29" [[projects]] digest = "1:1d8e1cb71c33a9470bbbae09bfec09db43c6bf358dfcae13cd8807c4e2a9a2bf" @@ -244,7 +244,7 @@ [[projects]] branch = "master" - digest = "1:e469cd65badf7694aeb44874518606d93c1d59e7735d3754ad442782437d3cc3" + digest = "1:63b68062b8968092eb86bedc4e68894bd096ea6b24920faca8b9dcf451f54bb5" name = "github.com/prometheus/common" packages = [ "expfmt", @@ -252,7 +252,7 @@ "model", ] pruneopts = "UT" - revision = "7600349dcfe1abd18d72d3a1770870d9800a7801" + revision = "c7de2306084e37d54b8be01f3541a8464345e9a5" [[projects]] branch = "master" @@ -418,14 +418,14 @@ [[projects]] branch = "master" - digest = "1:12ff7b51d336ea7e8b182aa3313679a37d53de64f84d2c3cbfd6a0237877e20a" + digest = "1:bb0fe59917bdd5b89f49b9a8b26e5f465e325d9223b3a8e32254314bdf51e0f1" name = "golang.org/x/sys" packages = [ "cpu", "unix", ] pruneopts = "UT" - revision = "e072cadbbdc8dd3d3ffa82b8b4b9304c261d9311" + revision = "3dc4335d56c789b04b0ba99b7a37249d9b614314" [[projects]] digest = "1:a2ab62866c75542dd18d2b069fec854577a20211d7c0ea6ae746072a1dccdd18" @@ -451,11 +451,12 @@ version = "v0.3.0" [[projects]] - digest = "1:cd018653a358d4b743a9d3bee89e825521f2ab2f2ec0770164bf7632d8d73ab7" + branch = "master" + digest = "1:077c1c599507b3b3e9156d17d36e1e61928ee9b53a5b420f10f28ebd4a0b275c" name = "google.golang.org/genproto" packages = ["googleapis/rpc/status"] pruneopts = "UT" - revision = "7fd901a49ba6a7f87732eb344f6e3c5b19d1b200" + revision = "daca94659cb50e9f37c1b834680f2e46358f10b0" [[projects]] digest = "1:2dab32a43451e320e49608ff4542fdfc653c95dcc35d0065ec9c6c3dd540ed74" diff --git a/Gopkg.toml b/Gopkg.toml index cf0d4eceb..9d605f6cc 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -72,20 +72,6 @@ ## Some repos dont have releases. ## Pin to revision -## We can remove this one by updating protobuf to v1.1.0 -## but then the grpc tests break with -#--- FAIL: TestBroadcastTx (0.01s) -#panic: message/group field common.KVPair:bytes without pointer [recovered] -# panic: message/group field common.KVPair:bytes without pointer -# -# ... -# -# github.com/tendermint/tendermint/rpc/grpc_test.TestBroadcastTx(0xc420a5ab40) -# /go/src/github.com/tendermint/tendermint/rpc/grpc/grpc_test.go:29 +0x141 -[[override]] - name = "google.golang.org/genproto" - revision = "7fd901a49ba6a7f87732eb344f6e3c5b19d1b200" - [[override]] name = "github.com/jmhodges/levigo" revision = "c42d9e0ca023e2198120196f842701bb4c55d7b9" From a040c36dfb262e2d7150289eb1dd896eccdf13cf Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Thu, 2 Aug 2018 10:43:47 -0700 Subject: [PATCH 13/19] (squash this) change adr number, remove redundancy in function names --- ...mmetric-crypto.md => adr-013-symmetric-crypto.md} | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename docs/architecture/{adr-015-symmetric-crypto.md => adr-013-symmetric-crypto.md} (85%) diff --git a/docs/architecture/adr-015-symmetric-crypto.md b/docs/architecture/adr-013-symmetric-crypto.md similarity index 85% rename from docs/architecture/adr-015-symmetric-crypto.md rename to docs/architecture/adr-013-symmetric-crypto.md index 7a587d18a..00442ab0d 100644 --- a/docs/architecture/adr-015-symmetric-crypto.md +++ b/docs/architecture/adr-013-symmetric-crypto.md @@ -1,4 +1,4 @@ -# ADR 015: Need for symmetric cryptography +# ADR 013: Need for symmetric cryptography ## Context @@ -31,9 +31,9 @@ Consequently decryption with multiple algoritms is sub-optimal. We should create the following two methods in a new `crypto/encoding/symmetric` package: ```golang -func EncryptSymmetric(aead cipher.AEAD, plaintext []byte) (ciphertext []byte, err error) -func DecryptSymmetric(key []byte, ciphertext []byte) (plaintext []byte, err error) -func RegisterSymmetric(aead cipher.AEAD, algo_name string, NewAead func(key []byte) (cipher.Aead, error)) error +func Encrypt(aead cipher.AEAD, plaintext []byte) (ciphertext []byte, err error) +func Decrypt(key []byte, ciphertext []byte) (plaintext []byte, err error) +func Register(aead cipher.AEAD, algo_name string, NewAead func(key []byte) (cipher.Aead, error)) error ``` This allows you to specify the algorithm in encryption, but not have to specify @@ -43,7 +43,7 @@ looking at the file directly. One downside is that for the encrypt function you must have already initialized an AEAD, but I don't really see this as an issue. -If there is no error in encryption, EncryptSymmetric will return `algo_name || nonce || aead_ciphertext`. +If there is no error in encryption, Encrypt will return `algo_name || nonce || aead_ciphertext`. `algo_name` should be length prefixed, using standard varuint encoding. This will be binary data, but thats not a problem considering the nonce and ciphertext are also binary. @@ -63,7 +63,7 @@ Then we maintain a map from the name returned from `getType(aead)` to `algo_name In decryption, we read the `algo_name`, and then instantiate a new AEAD with the key. Then we call the AEAD's decrypt method on the provided nonce/ciphertext. -`RegisterSymmetric` allows a downstream user to add their own desired AEAD to the symmetric package. +`Register` allows a downstream user to add their own desired AEAD to the symmetric package. It will error if the AEAD name is already registered. This prevents a malicious import from modifying / nullifying an AEAD at runtime. From 87f09adeec3c2c589d34a2902557fe4a92860a04 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Thu, 2 Aug 2018 23:27:16 -0700 Subject: [PATCH 14/19] (Squash this) Be more explicit about the exact encoding of the secp signature --- docs/architecture/adr-015-crypto-encoding.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-015-crypto-encoding.md b/docs/architecture/adr-015-crypto-encoding.md index 983411270..67cce95f9 100644 --- a/docs/architecture/adr-015-crypto-encoding.md +++ b/docs/architecture/adr-015-crypto-encoding.md @@ -59,8 +59,8 @@ Use the canonical representation for signatures. #### Secp256k1 There isn't a clear canonical representation here. Signatures have two elements `r,s`. -We should encode these bytes as `r || s`, where `r` and `s` are both exactly -32 bytes long. +These bytes are encoded as `r || s`, where `r` and `s` are both exactly +32 bytes long, encoded big-endian. This is basically Ethereum's encoding, but without the leading recovery bit. ## Status From d09a3a6d3a3bcc43f1007c323e57375d2a10d783 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 3 Aug 2018 11:24:55 +0400 Subject: [PATCH 15/19] stop gracefully instead of trying to resume ops Refs #2072 We most probably shouldn't be running any further when there is some unexpected panic. Some unknown error happened, and so we don't know if that will result in the validator signing an invalid thing. It might be worthwhile to explore a mechanism for manual resuming via some console or secure RPC system, but for now, halting the chain upon unexpected consensus bugs sounds like the better option. --- consensus/state.go | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/consensus/state.go b/consensus/state.go index 435427a91..6ffe6ef64 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -553,10 +553,30 @@ func (cs *ConsensusState) newStep() { // Updates (state transitions) happen on timeouts, complete proposals, and 2/3 majorities. // ConsensusState must be locked before any internal state is updated. func (cs *ConsensusState) receiveRoutine(maxSteps int) { + onExit := func(cs *ConsensusState) { + // NOTE: the internalMsgQueue may have signed messages from our + // priv_val that haven't hit the WAL, but its ok because + // priv_val tracks LastSig + + // close wal now that we're done writing to it + cs.wal.Stop() + cs.wal.Wait() + + close(cs.done) + } + defer func() { if r := recover(); r != nil { cs.Logger.Error("CONSENSUS FAILURE!!!", "err", r, "stack", string(debug.Stack())) - go cs.receiveRoutine(0) + // stop gracefully + // + // NOTE: We most probably shouldn't be running any further when there is + // some unexpected panic. Some unknown error happened, and so we don't + // know if that will result in the validator signing an invalid thing. It + // might be worthwhile to explore a mechanism for manual resuming via + // some console or secure RPC system, but for now, halting the chain upon + // unexpected consensus bugs sounds like the better option. + onExit(cs) } }() @@ -589,15 +609,7 @@ func (cs *ConsensusState) receiveRoutine(maxSteps int) { // go to the next step cs.handleTimeout(ti, rs) case <-cs.Quit(): - // NOTE: the internalMsgQueue may have signed messages from our - // priv_val that haven't hit the WAL, but its ok because - // priv_val tracks LastSig - - // close wal now that we're done writing to it - cs.wal.Stop() - cs.wal.Wait() - - close(cs.done) + onExit(cs) return } } From b1cff0f9bffd4d88ca2c6e9b9bc8e6d44cd3b463 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 3 Aug 2018 11:34:58 +0400 Subject: [PATCH 16/19] [libs/autofile] create a Group ticker on Start 1) no need to stop the ticker in createTestGroup() method 2) now there is a symmetry - we start the ticker in OnStart(), we stop it in OnStop() Refs #2072 --- libs/autofile/group.go | 2 +- libs/autofile/group_test.go | 34 ++++++++++++++++++---------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/libs/autofile/group.go b/libs/autofile/group.go index 9b78c5110..e747f04dd 100644 --- a/libs/autofile/group.go +++ b/libs/autofile/group.go @@ -85,7 +85,6 @@ func OpenGroup(headPath string) (g *Group, err error) { Head: head, headBuf: bufio.NewWriterSize(head, 4096*10), Dir: dir, - ticker: time.NewTicker(groupCheckDuration), headSizeLimit: defaultHeadSizeLimit, totalSizeLimit: defaultTotalSizeLimit, minIndex: 0, @@ -102,6 +101,7 @@ func OpenGroup(headPath string) (g *Group, err error) { // OnStart implements Service by starting the goroutine that checks file and // group limits. func (g *Group) OnStart() error { + g.ticker = time.NewTicker(groupCheckDuration) go g.processTicks() return nil } diff --git a/libs/autofile/group_test.go b/libs/autofile/group_test.go index d6b10a420..d87bdba82 100644 --- a/libs/autofile/group_test.go +++ b/libs/autofile/group_test.go @@ -16,23 +16,25 @@ import ( cmn "github.com/tendermint/tendermint/libs/common" ) -// NOTE: Returned group has ticker stopped -func createTestGroup(t *testing.T, headSizeLimit int64) *Group { +func createTestGroupWithHeadSizeLimit(t *testing.T, headSizeLimit int64) *Group { testID := cmn.RandStr(12) testDir := "_test_" + testID err := cmn.EnsureDir(testDir, 0700) require.NoError(t, err, "Error creating dir") + headPath := testDir + "/myfile" g, err := OpenGroup(headPath) require.NoError(t, err, "Error opening Group") - g.SetHeadSizeLimit(headSizeLimit) - g.ticker.Stop() require.NotEqual(t, nil, g, "Failed to create Group") + + g.SetHeadSizeLimit(headSizeLimit) + return g } func destroyTestGroup(t *testing.T, g *Group) { g.Close() + err := os.RemoveAll(g.Dir) require.NoError(t, err, "Error removing test Group directory") } @@ -45,7 +47,7 @@ func assertGroupInfo(t *testing.T, gInfo GroupInfo, minIndex, maxIndex int, tota } func TestCheckHeadSizeLimit(t *testing.T) { - g := createTestGroup(t, 1000*1000) + g := createTestGroupWithHeadSizeLimit(t, 1000*1000) // At first, there are no files. assertGroupInfo(t, g.ReadGroupInfo(), 0, 0, 0, 0) @@ -107,7 +109,7 @@ func TestCheckHeadSizeLimit(t *testing.T) { } func TestSearch(t *testing.T) { - g := createTestGroup(t, 10*1000) + g := createTestGroupWithHeadSizeLimit(t, 10*1000) // Create some files in the group that have several INFO lines in them. // Try to put the INFO lines in various spots. @@ -208,7 +210,7 @@ func TestSearch(t *testing.T) { } func TestRotateFile(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) g.WriteLine("Line 1") g.WriteLine("Line 2") g.WriteLine("Line 3") @@ -238,7 +240,7 @@ func TestRotateFile(t *testing.T) { } func TestFindLast1(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) g.WriteLine("Line 1") g.WriteLine("Line 2") @@ -262,7 +264,7 @@ func TestFindLast1(t *testing.T) { } func TestFindLast2(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) g.WriteLine("Line 1") g.WriteLine("Line 2") @@ -286,7 +288,7 @@ func TestFindLast2(t *testing.T) { } func TestFindLast3(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) g.WriteLine("Line 1") g.WriteLine("# a") @@ -310,7 +312,7 @@ func TestFindLast3(t *testing.T) { } func TestFindLast4(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) g.WriteLine("Line 1") g.WriteLine("Line 2") @@ -332,7 +334,7 @@ func TestFindLast4(t *testing.T) { } func TestWrite(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) written := []byte("Medusa") g.Write(written) @@ -353,7 +355,7 @@ func TestWrite(t *testing.T) { // test that Read reads the required amount of bytes from all the files in the // group and returns no error if n == size of the given slice. func TestGroupReaderRead(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) professor := []byte("Professor Monster") g.Write(professor) @@ -382,7 +384,7 @@ func TestGroupReaderRead(t *testing.T) { // test that Read returns an error if number of bytes read < size of // the given slice. Subsequent call should return 0, io.EOF. func TestGroupReaderRead2(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) professor := []byte("Professor Monster") g.Write(professor) @@ -413,7 +415,7 @@ func TestGroupReaderRead2(t *testing.T) { } func TestMinIndex(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) assert.Zero(t, g.MinIndex(), "MinIndex should be zero at the beginning") @@ -422,7 +424,7 @@ func TestMinIndex(t *testing.T) { } func TestMaxIndex(t *testing.T) { - g := createTestGroup(t, 0) + g := createTestGroupWithHeadSizeLimit(t, 0) assert.Zero(t, g.MaxIndex(), "MaxIndex should be zero at the beginning") From 2878c7523f7f7b516aee1e3d09e0215223e47631 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 3 Aug 2018 11:39:57 +0400 Subject: [PATCH 17/19] update github bug report template (#2131) --- .github/ISSUE_TEMPLATE/bug-report.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md index 7ace32f24..9c56a364a 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.md +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -1,9 +1,9 @@ --- -name: Bug Report +name: Bug Report about: Create a report to help us squash bugs! --- -