This issue is related to #3107
This is a first renaming/refactoring step before reworking and removing heartbeats.
As discussed with @Liamsi , we preferred to go for a couple of independent and separate PRs to simplify review work.
The changes:
Help to clarify the relation between the validator and remote signer endpoints
Differentiate between timeouts and deadlines
Prepare to encapsulate networking related code behind RemoteSigner in the next PR
My intention is to separate and encapsulate the "network related" code from the actual signer.
SignerRemote ---(uses/contains)--> SignerValidatorEndpoint <--(connects to)--> SignerServiceEndpoint ---> SignerService (future.. not here yet but would like to decouple too)
All reconnection/heartbeat/whatever code goes in the endpoints. Signer[Remote/Service] do not need to know about that.
I agree Endpoint may not be the perfect name. I tried to find something "Go-ish" enough. It is a common name in go-kit, kubernetes, etc.
Right now:
SignerValidatorEndpoint:
handles the listener
contains SignerRemote
Implements the PrivValidator interface
connects and sets a connection object in a contained SignerRemote
delegates PrivValidator some calls to SignerRemote which in turn uses the conn object that was set externally
SignerRemote:
Implements the PrivValidator interface
read/writes from a connection object directly
handles heartbeats
SignerServiceEndpoint:
Does most things in a single place
delegates to a PrivValidator IIRC.
* cleanup
* Refactoring step 1
* Refactoring step 2
* move messages to another file
* mark for future work / next steps
* mark deprecated classes in docs
* Fix linter problems
* additional linter fixes
* docs: explain create_empty_blocks configurations
Closes#3307
* Vagrantfile: install nodejs for docs
* update docs instructions
npm install does not make sense since there's no packages.json file
* explain broadcast_tx_* tx format
Closes#536
* docs: explain how transaction ordering works
Closes#2904
* bring in consensus parameters explained
* example for create_empty_blocks_interval
* bring in explanation from https://github.com/tendermint/tendermint/issues/2487#issuecomment-424899799
* link to formatting instead of duplicating info
* libs/common: TrapSignal accepts logger as a first parameter
and does not block anymore
* previously it was dumping "captured ..." msg to os.Stdout
* TrapSignal should not be responsible for blocking thread of execution
Refs #3238
* exit with zero (0) code upon receiving SIGTERM/SIGINT
Refs #3238
* fix formatting in docs/app-dev/abci-cli.md
Co-Authored-By: melekes <anton.kalyaev@gmail.com>
* fix formatting in docs/app-dev/abci-cli.md
Co-Authored-By: melekes <anton.kalyaev@gmail.com>
* bound mempool memory usage
Closes#3079
* rename SizeBytes to TxsTotalBytes
and other small fixes after Zarko's review
* rename MaxBytes to MaxTxsTotalBytes
* make ErrMempoolIsFull more informative
* expose mempool's txs_total_bytes via RPC
* test full response
* fixes after Ethan's review
* config: rename mempool.size to mempool.max_txs
https://github.com/tendermint/tendermint/pull/3248#discussion_r254034004
* test more cases
https://github.com/tendermint/tendermint/pull/3248#discussion_r254036532
* simplify test
* Revert "config: rename mempool.size to mempool.max_txs"
This reverts commit 39bfa36961.
* rename count back to n_txs
to make a change non-breaking
* rename max_txs_total_bytes to max_txs_bytes
* format code
* fix TestWALPeriodicSync
The test was sometimes failing due to processFlushTicks being called too
early. The solution is to call wal#Start later in the test.
* Apply suggestions from code review
* reject the shared secret if is all zeros in case the blacklist was not
sufficient
* Add test that verifies lower order pub-keys are rejected at the DH step
* Update changelog
* fix typo in test-comment
* green pubsub tests :OK:
* get rid of clientToQueryMap
* Subscribe and SubscribeUnbuffered
* start adapting other pkgs to new pubsub
* nope
* rename MsgAndTags to Message
* remove TagMap
it does not bring any additional benefits
* bring back EventSubscriber
* fix test
* fix data race in TestStartNextHeightCorrectly
```
Write at 0x00c0001c7418 by goroutine 796:
github.com/tendermint/tendermint/consensus.TestStartNextHeightCorrectly()
/go/src/github.com/tendermint/tendermint/consensus/state_test.go:1296 +0xad
testing.tRunner()
/usr/local/go/src/testing/testing.go:827 +0x162
Previous read at 0x00c0001c7418 by goroutine 858:
github.com/tendermint/tendermint/consensus.(*ConsensusState).addVote()
/go/src/github.com/tendermint/tendermint/consensus/state.go:1631 +0x1366
github.com/tendermint/tendermint/consensus.(*ConsensusState).tryAddVote()
/go/src/github.com/tendermint/tendermint/consensus/state.go:1476 +0x8f
github.com/tendermint/tendermint/consensus.(*ConsensusState).handleMsg()
/go/src/github.com/tendermint/tendermint/consensus/state.go:667 +0xa1e
github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine()
/go/src/github.com/tendermint/tendermint/consensus/state.go:628 +0x794
Goroutine 796 (running) created at:
testing.(*T).Run()
/usr/local/go/src/testing/testing.go:878 +0x659
testing.runTests.func1()
/usr/local/go/src/testing/testing.go:1119 +0xa8
testing.tRunner()
/usr/local/go/src/testing/testing.go:827 +0x162
testing.runTests()
/usr/local/go/src/testing/testing.go:1117 +0x4ee
testing.(*M).Run()
/usr/local/go/src/testing/testing.go:1034 +0x2ee
main.main()
_testmain.go:214 +0x332
Goroutine 858 (running) created at:
github.com/tendermint/tendermint/consensus.(*ConsensusState).startRoutines()
/go/src/github.com/tendermint/tendermint/consensus/state.go:334 +0x221
github.com/tendermint/tendermint/consensus.startTestRound()
/go/src/github.com/tendermint/tendermint/consensus/common_test.go:122 +0x63
github.com/tendermint/tendermint/consensus.TestStateFullRound1()
/go/src/github.com/tendermint/tendermint/consensus/state_test.go:255 +0x397
testing.tRunner()
/usr/local/go/src/testing/testing.go:827 +0x162
```
* fixes after my own review
* fix formatting
* wait 100ms before kicking a subscriber out
+ a test for indexer_service
* fixes after my second review
* no timeout
* add changelog entries
* fix merge conflicts
* fix typos after Thane's review
Co-Authored-By: melekes <anton.kalyaev@gmail.com>
* reformat code
* rewrite indexer service in the attempt to fix failing test
https://github.com/tendermint/tendermint/pull/3227/#issuecomment-462316527
* Revert "rewrite indexer service in the attempt to fix failing test"
This reverts commit 0d9107a098.
* another attempt to fix indexer
* fixes after Ethan's review
* use unbuffered channel when indexing transactions
Refs https://github.com/tendermint/tendermint/pull/3227#discussion_r258786716
* add a comment for EventBus#SubscribeUnbuffered
* format code
As per #3043, this adds a ticker to sync the WAL every 2s while the WAL is running.
* Flush WAL every 2s
This adds a ticker that flushes the WAL every 2s while the WAL is
running. This is related to #3043.
* Fix spelling
* Increase timeout to 2mins for slower build environments
* Make WAL sync interval configurable
* Add TODO to replace testChan with more comprehensive testBus
* Remove extraneous debug statement
* Remove testChan in favour of using system time
As per
https://github.com/tendermint/tendermint/pull/3300#discussion_r255886586,
this removes the `testChan` WAL member and replaces the approach with a
system time-oriented one. In this new approach, we keep track of the
system time at which each flush and periodic flush successfully
occurred.
The naming of the various functions is also updated here to be more
consistent with "flushing" as opposed to "sync'ing".
* Update naming convention and ensure lock for timestamp update
* Add Flush method as part of WAL interface
Adds a `Flush` method as part of the WAL interface to enforce the idea
that we can manually trigger a WAL flush from outside of the WAL. This
is employed in the consensus state management to flush the WAL prior to
signing votes/proposals, as per https://github.com/tendermint/tendermint/issues/3043#issuecomment-453853630
* Update CHANGELOG_PENDING
* Remove mutex approach and replace with DI
The dependency injection approach to dealing with testing concerns could
allow similar effects to some kind of "testing bus"-based approach. This
commit introduces an example of this, where instead of relying on
(potentially fragile) timing of things between the code and the test, we
inject code into the function under test that can signal the test
through a channel.
This allows us to avoid the `time.Sleep()`-based approach previously
employed.
* Update comment on WAL flushing during vote signing
Co-Authored-By: thanethomson <connect@thanethomson.com>
* Simplify flush interval definition
Co-Authored-By: thanethomson <connect@thanethomson.com>
* Expand commentary on WAL disk flushing
Co-Authored-By: thanethomson <connect@thanethomson.com>
* Add broken test to illustrate WAL sync test problem
Removes test-related state (dependency injection code) from the WAL data
structure and adds test code to illustrate the problem with using
`WALGenerateNBlocks` and `wal.SearchForEndHeight` to test periodic
sync'ing.
* Fix test error messages
* Use WAL group buffer size to check for flush
A function is added to `libs/autofile/group.go#Group` in order to return
the size of the buffered data (i.e. data that has not yet been flushed
to disk). The test now checks that, prior to a `time.Sleep`, the group
buffer has data in it. After the `time.Sleep` (during which time the
periodic flush should have been called), the buffer should be empty.
* Remove config root dir removal from #3291
* Add godoc for NewWAL mentioning periodic sync
ref: [#3010 (comment)](https://github.com/tendermint/tendermint/issues/3010#issuecomment-464287627)
> I tried searching for code where we authenticate a peer against its NetAddress.ID and couldn't find it. I don't see a reason to switch to Noise, but a need to ensure that the node's ID is authenticated e.g. after dialing from the address book.
* p2p: check secret conn id matches dialed id
* Fix all p2p tests & make code compile
* add simple test for dialing with wrong ID
* update changelog
* address review comments
* yet another place where to use IDAddressString and fix
testSetupMultiplexTransport
* changelog: use issue number instead of PR number
* follow up to #3291
- rpc/test/helpers.go add StopTendermint(node) func
- remove ensureDir(filepath.Dir(walFile), 0700)
- mempool/mempool_test.go add type cleanupFunc func()
* cmd/show_validator: wrap err to make it more clear
* improve ResetTestRootWithChainID() concurrency safety
Rely on ioutil.TempDir() to create test root directories and ensure
multiple same-chain id test cases can run in parallel.
* Update config/toml.go
Co-Authored-By: alessio <quadrispro@ubuntu.com>
* clean up test directories after completion
Closes: #1034
* Remove redundant EnsureDir call
* s/PanicSafety()/panic()/s
* Put create dir functionality back in ResetTestRootWithChainID
* Place test directories in OS's tempdir
In modern UNIX and UNIX-like systems /tmp is very often
mounted as tmpfs. This might speed test execution a bit.
* Set 0700 to a const
* rootsDirs -> configRootDirs
* Don't double remove directories
* Avoid global variables
* Fix consensus tests
* Reduce defer stack
* Address review comments
* Try to fix tests
* Update CHANGELOG_PENDING.md
Co-Authored-By: alessio <quadrispro@ubuntu.com>
* Update consensus/common_test.go
Co-Authored-By: alessio <quadrispro@ubuntu.com>
* Update consensus/common_test.go
Co-Authored-By: alessio <quadrispro@ubuntu.com>
Earlier this week somebody posted this in GoS Riot chat:
```
E[2019-02-12|10:38:37.596] Corrupted entry. Skipping... module=consensus wal=/home/gaia/.gaiad/data/cs.wal/wal err="DataCorruptionError[length 878916964 exceeded maximum possible value of 1048576 bytes]"
E[2019-02-12|10:38:37.596] Corrupted entry. Skipping... module=consensus wal=/home/gaia/.gaiad/data/cs.wal/wal err="DataCorruptionError[length 825701731 exceeded maximum possible value of 1048576 bytes]"
E[2019-02-12|10:38:37.596] Corrupted entry. Skipping... module=consensus wal=/home/gaia/.gaiad/data/cs.wal/wal err="DataCorruptionError[length 1631073634 exceeded maximum possible value of 1048576 bytes]"
E[2019-02-12|10:38:37.596] Corrupted entry. Skipping... module=consensus wal=/home/gaia/.gaiad/data/cs.wal/wal err="DataCorruptionError[length 912418148 exceeded maximum possible value of 1048576 bytes]"
E[2019-02-12|10:38:37.600] Corrupted entry. Skipping... module=consensus wal=/home/gaia/.gaiad/data/cs.wal/wal err="DataCorruptionError[failed to read data: EOF]"
E[2019-02-12|10:38:37.600] Error on catchup replay. Proceeding to start ConsensusState anyway module=consensus err="Cannot replay height 7242. WAL does not contain #ENDHEIGHT for 7241"
E[2019-02-12|10:38:37.861] Error dialing peer module=p2p err="dial tcp 35.183.126.181:26656: i/o timeout
```
Note the length error messages. What has happened is the length field got corrupted probably. I've looked at the code and noticed that we don't check the msg size during encoding. This PR fixes that. It also improves a few error messages in WALDecoder.
* rpc/net_info: change RemoteIP type from net.IP to String
Before:
"AAAAAAAAAAAAAP//rB8ktw=="
which is amino-encoded net.IP byte slice
After:
"192.0.2.1"
Fixes#3251
* rpc/net_info: non-empty response in docs
* not related to linter: remove obsolete constants:
- `Insecure` and `Secure` and type `Security` are not used anywhere
* not related to linter: update example
- NewInsecure was deleted; change example to NewRemoteDB
* address: Binds to all network interfaces (gosec):
- bind to localhost instead of 0.0.0.0
- regenerate test key and cert for this purpose (was valid for ::) and
otherwise we would see:
transport: authentication handshake failed: x509: certificate is
valid for ::, not 127.0.0.1\"
(used https://github.com/google/keytransparency/blob/master/scripts/gen_server_keys.sh
to regenerate certs)
* use sha256 in tests instead of md5; time difference is negligible
* nolint usage of math/rand in test and add comment on its import
- crypto/rand is slower and we do not need sth more secure in tests
* enable linter in circle-ci
* another nolint math/rand in test
* replace another occurrence of md5
* consistent comment about importing math/rand
* types.NewCommit
* use types.NewCommit everywhere
* fix log in unsafe_reset
* memoize height and round in constructor
* notes about deprecating toVote
* bring back memoizeHeightRound
* evidence: NewEvidencePool takes evidenceDB
* evidence: failing TestStoreCommitDuplicate
tendermint/security#35
* GetEvidence -> GetEvidenceInfo
* fix TestStoreCommitDuplicate
* comment in VerifyEvidence
* add check if evidence was already seen
- modify EventPool interface (EventStore is not known in ApplyBlock):
- add IsCommitted method to iface
- add test
* update changelog
* fix TestStoreMark:
- priority in evidence info gets reset to zero after evidence gets committed
* review comments: simplify EvidencePool.IsCommitted
- delete obsolete EvidenceStore.IsCommitted
* add simple test for IsCommitted
* update changelog: this is actually breaking (PR number still missing)
* fix TestStoreMark:
- priority in evidence info gets reset to zero after evidence gets
committed
* review suggestion: simplify return
* Initial commit for 3181..still early
* unit test updates
* unit test updates
* fix check of dups accross updates and deletes
* simplify the processChange() func
* added overflow check utest
* Added checks for empty valset, new utest
* deepcopy changes in processUpdate()
* moved to new API, fixed tests
* test cleanup
* address review comments
* make sure votePower > 0
* gofmt fixes
* handle duplicates and invalid values
* more work on tests, review comments
* Renamed and explained K
* make TestVal private
* split verifyUpdatesAndComputeNewPriorities.., added check for deletes
* return error if validator set is empty after processing changes
* address review comments
* lint err
* Fixed the total voting power and added comments
* fix lint
* fix lint
* failing test
* fix infinite loop in addrbook
There are cases where we only have a small number of addresses marked
good ("old"), but the selection mechanism keeps trying to select more of these
addresses, and hence ends up in an infinite loop. Here we fix this to
only try and select such "old" addresses if we have enough of them. Note this
means, if we don't have enough of them, we may return more "new"
addresses than otherwise expected by the newSelectionBias.
This whole GetSelectionWithBias method probably needs to be rewritten,
but this is a quick fix for the issue.
* changelog
* fix infinite loop if not enough new addrs
* fix another potential infinite loop
if a.nNew == 0 -> pickFromOldBucket=true, but we don't have enough items
(a.nOld > len(oldBucketToAddrsMap) false)
* Revert "fix another potential infinite loop"
This reverts commit 146540c112.
* check num addresses instead of buckets, new test
* fixed the int division
* add slack to bias % in test, lint fixes
* Added checks for selection content in test
* test cleanup
* Apply suggestions from code review
Co-Authored-By: ebuchman <ethan@coinculture.info>
* address review comments
* change after Anton's review comments
* use the same docker image we use for testing
when building a binary for localnet
* switch back to circleci classic
* more review comments
* more review comments
* refactor addrbook_test
* build linux binary inside docker
in attempt to fix
```
--> Running dep
+ make build-linux
GOOS=linux GOARCH=amd64 make build
make[1]: Entering directory `/home/circleci/.go_workspace/src/github.com/tendermint/tendermint'
CGO_ENABLED=0 go build -ldflags "-X github.com/tendermint/tendermint/version.GitCommit=`git rev-parse --short=8 HEAD`" -tags 'tendermint' -o build/tendermint ./cmd/tendermint/
p2p/pex/addrbook.go:373:13: undefined: math.Round
```
* change dir from /usr to /go
* use concrete Go version for localnet binary
* check for nil addresses just to be sure
* WIP: Starts adding remote signer test harness
This commit adds a new command to Tendermint to allow for us to build a
standalone binary to test remote signers such as KMS
(https://github.com/tendermint/kms).
Right now, all it does is test that the local public key matches the
public key reported by the client, and fails at the point where it
attempts to get the client to sign a proposal.
* Fixes typo
* Fixes proposal validation test
This commit fixes the proposal validation test as per #3149. It also
moves the test harness into its own internal package to isolate its
exports from the `privval` package.
* Adds vote signing validation
* Applying recommendations from #3149
* Adds function descriptions for test harness
* Adds ability to ask remote signer to shut down
Prior to this commit, the remote signer needs to manually be shut down,
which is not ideal for automated testing. This commit allows us to send
a poison pill message to the KMS to let it shut down gracefully once
testing is done (whether the tests pass or fail).
* Adds tests for remote signer test harness
This commit makes some minor modifications to a few files to allow for
testing of the remote signer test harness. Two tests are added here:
checking for a fully successful (the ideal) case, and for the case where
the maximum number of retries has been reached when attempting to accept
incoming connections from the remote signer.
* Condenses serialization of proposals and votes using existing Tendermint functions
* Removes now-unnecessary amino import and codec
* Adds error message for vote signing failure
* Adds key extraction command for integration test
Took the code from here:
https://gist.github.com/Liamsi/a80993f24bff574bbfdbbfa9efa84bc7 to
create a simple utility command to extract a key from a local Tendermint
validator for use in KMS integration testing.
* Makes path expansion success non-compulsory
* Fixes segfault on SIGTERM
We need an additional variable to keep track of whether we're
successfully connected, otherwise hitting Ctrl+Break during execution
causes a segmentation fault. This now allows for a clean shutdown.
* Consolidates shutdown checks
* Adds comments indicating codes for easy lookup
* Adds Docker build for remote signer harness
Updates the `DOCKER/build.sh` and `DOCKER/push.sh` files to allow one to
override the image name and Dockerfile using environment variables.
Updates the primary `Makefile` as well as the `DOCKER/Makefile` to allow
for building the `remote_val_harness` Docker image.
* Adds build_remote_val_harness_docker_image to .PHONY
* Removes remote signer poison pill messaging functionality
* Reduces fluff code in command line parsing
As per
https://github.com/tendermint/tendermint/pull/3149#pullrequestreview-196171788,
this reduces the amount of fluff code in the PR down to the bare
minimum.
* Fixes ordering of error check and info log
* Moves remove_val_harness cmd into tools folder
It seems to make sense to rather keep the remote signer test harness in
its own tool folder (now rather named `tm-signer-harness` to keep with
the tool naming convention). It is actually a separate tool, not meant
to be one of the core binaries, but supplementary and supportive.
* Updates documentation for tm-signer-harness
* Refactors flag parsing to be more compact and less redundant
* Adds version sub-command help
* Removes extraneous flags parsing
* Adds CHANGELOG_PENDING entry for tm-signer-harness
* Improves test coverage
Adds a few extra parameters to the `MockPV` type to fake broken vote and
proposal signing. Also adds some more tests for the test harness so as
to increase coverage for failed cases.
* Fixes formatting for CHANGELOG_PENDING.md
* Fix formatting for documentation config
* Point users towards official Tendermint docs for tools documentation
* Point users towards official Tendermint docs for tm-signer-harness
* Remove extraneous constant
* Rename TestHarness.sc to TestHarness.spv for naming consistency
* Refactor to remove redundant goroutine
* Refactor conditional to cleaner switch statement and better error handling for listener protocol
* Remove extraneous goroutine
* Add note about installing tmkms via Cargo
* Fix typo in naming of output signing key
* Add note about where to find chain ID
* Replace /home/user with ~/ for brevity
* Fixes "signer.key" typo
* Minor edits for clarification for tm-signer-harness bulid/setup process
Juan Leni
Anton Kaliaev
Ismail Khoffi
Ethan Buchman
Daniil Lashin
Thane Thomson
Alessio Treglia
Zarko Milosevic
Alexander Bezobchuk
Zach
Anca Zamfir