found out this issue when trying to decouple mempool reactor with its
underlying clist implementation.
according to its comment, the test `TestReactorNoBroadcastToSender` is
intended to make sure that a peer shouldn't send tx back to its origin.
however, the current test forgot to init peer state key, thus the code
will get stuck at waiting for peer to catch up state *instead of* skipping
sending tx back:
b8d08b9ef4/mempool/reactor.go (L216-L226)
this PR fixes the issue by init peer state key.
## Description
Add test vectors for all reactors
- [x] state-sync
- [x] privval
- [x] mempool
- [x] p2p
- [x] evidence
- [ ] light?
this PR is primarily oriented at testvectors for things going over the wire. should we expand the testvectors into types as well?
Closes: #XXX
## Description
This PR wraps the stdlib sync.(RW)Mutex & godeadlock.(RW)Mutex. This enables using go-deadlock via a build flag instead of using sed to replace sync with godeadlock in all files
Closes: #3242
In order to have more control over the mempool implementation,
introduce a new exported function RemoveTxByKey.
Export also TxKey() and TxKeySize. Use TxKeySize const instead of
sha256.size, so future changes on the hash function won't break the API.
Allows using a TxKey (32 bytes reference) as parameter instead of
the complete array set. So the application layer does not need to
keep track of the whole transaction but only of the sha256 hash (32 bytes).
This function is useful when mempool.Recheck is disabled.
Allows the Application layer to implement its own cleaning mechanism
without having to re-implement the whole mempool interface.
Mempool.Update() would probably also need to change from txBytes to txKey,
but that would require to change the Interface thus will break backwards
compatibility. For now RemoveTxByKey() looks like a good compromise,
it won't break anything and will help to solve some mempool issues from the
application layer.
Signed-off-by: p4u <pau@dabax.net>
## Description
To provide the ability to add more message types without needing to cause a breaking change the mempool message was migrated to a oneof.
Closes: #XXX
* proto: move mempool to proto
- changes according to moving the mempool reactor to proto
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
Closes: #2883
Closes#4603
Commands used (VIM):
```
:args `rg -l errors.Wrap`
:argdo normal @q | update
```
where q is a macros rewriting the `errors.Wrap` to `fmt.Errorf`.
to prevent malicious nodes from sending us large messages (~21MB, which
is the default `RecvMessageCapacity`)
This allows us to remove unnecessary `maxMsgSize` check in `decodeMsg`. Since each channel has a msg capacity set to `maxMsgSize`, there's no need to check it again in `decodeMsg`.
Closes#1503
Previously, many reactors were initialized with the name "Reactor," which made it difficult to log which reactor was doing what. This changes those reactors' names to something more descriptive.
* lint: golint issue fixes
- on my local machine golint is a lot stricter than the bot so slowly going through and fixing things.
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* more fixes from golint
* remove isPeerPersistentFn
* add changelog entry
* libs/common: Refactor libs/common 5
- move mathematical functions and types out of `libs/common` to math pkg
- move net functions out of `libs/common` to net pkg
- move string functions out of `libs/common` to strings pkg
- move async functions out of `libs/common` to async pkg
- move bit functions out of `libs/common` to bits pkg
- move cmap functions out of `libs/common` to cmap pkg
- move os functions out of `libs/common` to os pkg
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* fix testing issues
* fix tests
closes#41417
woooooooooooooooooo kill the cmn pkg
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* add changelog entry
* fix goimport issues
* run gofmt
* libs/common: refactor libs common 3
- move nil.go into types folder and make private
- move service & baseservice out of common into service pkg
ref #4147
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* add changelog entry
* libs/common: refactor libs/common 2
- move random function to there own pkg
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* change imports and usage throughout repo
* fix goimports
* add changelog entry
* Fix long line errors in abci, crypto, and libs packages
* Fix long lines in p2p and rpc packages
* Fix long lines in abci, state, and tools packages
* Fix long lines in behaviour and blockchain packages
* Fix long lines in cmd and config packages
* Begin fixing long lines in consensus package
* Finish fixing long lines in consensus package
* Add lll exclusion for lines containing URLs
* Fix long lines in crypto package
* Fix long lines in evidence package
* Fix long lines in mempool and node packages
* Fix long lines in libs package
* Fix long lines in lite package
* Fix new long line in node package
* Fix long lines in p2p package
* Ignore gocritic warning
* Fix long lines in privval package
* Fix long lines in rpc package
* Fix long lines in scripts package
* Fix long lines in state package
* Fix long lines in tools package
* Fix long lines in types package
* Enable lll linter
* Include sender when logging rejected txns
* Log as peerID to be consistent with other log messages
* Updated CHANGELOG_PENDING
* Handle nil source
* Updated PR link in CHANGELOG_PENDING
* Renamed TxInfo.SenderAddress and peerAddress til PeerFullID
* Renamed PeerFullID to PeerP2PID
* Forgot to rename a couple of references
* Correct memory alignment for 32-bit machine.
Switching the `txsBytes` and `rechecking` fields of `CListMempool` to ensure the correct memory alignment for `atomic.LoadInt64` on 32-bit machine.
* Update CHANGELOG_PENDING.md for #3968Fixed#3968 `mempool` Memory Loading Error on 32-bit Ubuntu 16.04 Machine.
* Update CHANGELOG_PENDING.md
Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
* Remove unnecessary type conversions
* Consolidate repeated strings into consts
* Clothe return statements
* Update blockchain/v1/reactor_fsm_test.go
Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
This PR repairs linter errors seen when running the following commands:
golangci-lint run --no-config --disable-all=true --enable=unconvert
golangci-lint run --no-config --disable-all=true --enable=goconst
golangci-lint run --no-config --disable-all=true --enable=nakedret
Contributes to #3262
Add gocritic as a linter
The linting is not complete, but should i complete in this PR or in a following.
23 files have been touched so it may be better to do in a following PR
Commits:
* Add gocritic to linting
- Added gocritic to linting
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* gocritic
* pr comments
* remove switch in cmdBatch
cleanup to add linter
grpc change:
https://godoc.org/google.golang.org/grpc#WithContextDialerhttps://godoc.org/google.golang.org/grpc#WithDialer
grpc/grpc-go#2627
prometheous change:
due to UninstrumentedHandler, being deprecated in the future
empty branch = empty if or else statement
didn't delete them entirely but commented
couldn't find a reason to have them
could not replicate the issue #3406
but if want to keep it commented then we should comment out the if statement as well
* Renamed wire.go to codec.go
- Wire was the previous name of amino
- Codec describes the file better than `wire` & `amino`
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* ide error
* rename amino.go to codec.go
As per #2127, this refactors the RequestCheckTx ProtoBuf struct to allow for a flag indicating whether a query is a recheck or not (and allows for possible future, more nuanced states).
In order to pass this extended information through to the ABCI app, the proxy.AppConnMempool (and, for consistency, the proxy.AppConnConsensus) interface seems to need to be refactored along with abcicli.Client.
And, as per this comment, I've made the following modification to the protobuf definition for the RequestCheckTx structure:
enum CheckTxType {
New = 0;
Recheck = 1;
}
message RequestCheckTx {
bytes tx = 1;
CheckTxType type = 2;
}
* Refactor ABCI CheckTx to notify of recheck
As per #2127, this refactors the `RequestCheckTx` ProtoBuf struct to allow for:
1. a flag indicating whether a query is a recheck or not (and allows for
possible future, more nuanced states)
2. an `additional_data` bytes array to provide information for those more
nuanced states.
In order to pass this extended information through to the ABCI app, the
`proxy.AppConnMempool` (and, for consistency, the
`proxy.AppConnConsensus`) interface seems to need to be refactored.
Commits:
* Fix linting issue
* Add CHANGELOG_PENDING entry
* Remove extraneous explicit initialization
* Update ABCI spec doc to include new CheckTx params
* Rename method param for consistency
* Rename CheckTxType enum values and remove additional_data param
* Refactor signature of Application.CheckTx
* Refactor signature of Application.DeliverTx
* Refactor example variable names for clarity and consistency
* Rename method variables for consistency
* Rename method variables for consistency
* add a changelog entry
* update docs
* mempool: remove only valid (Code==0) txs on Update
so evil proposers can't drop valid txs in Commit stage.
Also remove invalid (Code!=0) txs from the cache so they can be
resubmitted.
Fixes#3322@rickyyangz:
In the end of commit stage, we will update mempool to remove all the txs
in current block.
// Update mempool.
err = blockExec.mempool.Update(
block.Height,
block.Txs,
TxPreCheck(state),
TxPostCheck(state),
)
Assum an account has 3 transactions in the mempool, the sequences are
100, 101 and 102 separately, So an evil proposal can only package the
101 and 102 transactions into its proposal block, and leave 100 still in
mempool, then the two txs will be removed from all validators' mempool
when commit. So the account lost the two valid txs.
@ebuchman:
In the longer term we may want to do something like #2639 so we can
validate txs before we commit the block. But even in this case we'd only
want to run the equivalent of CheckTx, which means the DeliverTx could
still fail even if the CheckTx passes depending on how the app handles
the ABCI Code semantics. So more work will be required around the ABCI
code. See also #2185
* add changelog entry and tests
* improve changelog message
* reformat code
## Description
Refs #2659
Breaking changes in the mempool package:
[mempool] #2659 Mempool now an interface
old Mempool renamed to CListMempool
NewMempool renamed to NewCListMempool
Option renamed to CListOption
MempoolReactor renamed to Reactor
NewMempoolReactor renamed to NewReactor
unexpose TxID method
TxInfo.PeerID renamed to SenderID
unexpose MempoolReactor.Mempool
Breaking changes in the state package:
[state] #2659 Mempool interface moved to mempool package
MockMempool moved to top-level mock package and renamed to Mempool
Non Breaking changes in the node package:
[node] #2659 Add Mempool method, which allows you to access mempool
## Commits
* move Mempool interface into mempool package
Refs #2659
Breaking changes in the mempool package:
- Mempool now an interface
- old Mempool renamed to CListMempool
Breaking changes to state package:
- MockMempool moved to mempool/mock package and renamed to Mempool
- Mempool interface moved to mempool package
* assert CListMempool impl Mempool
* gofmt code
* rename MempoolReactor to Reactor
- combine everything into one interface
- rename TxInfo.PeerID to TxInfo.SenderID
- unexpose MempoolReactor.Mempool
* move mempool mock into top-level mock package
* add a fixme
TxsFront should not be a part of the Mempool interface
because it leaks implementation details. Instead, we need to come up
with general interface for querying the mempool so the MempoolReactor
can fetch and broadcast txs to peers.
* change node#Mempool to return interface
* save commit = new reactor arch
* Revert "save commit = new reactor arch"
This reverts commit 1bfceacd9d.
* require CListMempool in mempool.Reactor
* add two changelog entries
* fixes after my own review
* quote interfaces, structs and functions
* fixes after Ismail's review
* make node's mempool an interface
* make InitWAL/CloseWAL methods a part of Mempool interface
* fix merge conflicts
* make node's mempool an interface
* mempool: add a safety check, write tests for mempoolIDs
and document 65536 limit in the mempool reactor spec
follow-up to https://github.com/tendermint/tendermint/pull/2778
* rename the test
* fixes after Ismail's review
Refs #3306, irisnet@fdbb676
I ran an irishub validator. After the validator node ran several days, I dump the whole goroutine stack. I found that there were hundreds of broadcastTxRoutine. However, the connected peer quantity was less than 30. So I belive that there must be broadcastTxRoutine leakage issue.
According to my analysis, I think the root cause of this issue locate in below code:
select {
case <-next.NextWaitChan():
// see the start of the for loop for nil check
next = next.Next()
case <-peer.Quit():
return
case <-memR.Quit():
return
}
As we know, if multiple paths are avaliable in the same time, then a random path will be selected. Suppose that next.NextWaitChan() and peer.Quit() are both avaliable, and next.NextWaitChan() is chosen.
// send memTx
msg := &TxMessage{Tx: memTx.tx}
success := peer.Send(MempoolChannel, cdc.MustMarshalBinaryBare(msg))
if !success {
time.Sleep(peerCatchupSleepIntervalMS * time.Millisecond)
continue
}
Then next will be non-empty and the peer send operation won't be success. As a result, this go routine will be track into infinite loop and won't be released.
My proposal is to check peer.Quit() and memR.Quit() in every loop no matter whether next is nil.
Closes#1798
This is done by making every mempool tx maintain a list of peers who its received the tx from. Instead of using the 20byte peer ID, it instead uses a local map from peerID to uint16 counter, so every peer adds 2 bytes. (Word aligned to probably make it 8 bytes)
This also required resetting the callback function on every CheckTx. This likely has performance ramifications for instruction caching. The actual setting operation isn't costly with the removal of defers in this PR.
* Make the mempool not gossip txs back to peers its received it from
* Fix adversarial memleak
* Don't break interface
* Update changelog
* Forgot to add a mtx
* forgot a mutex
* Update mempool/reactor.go
Co-Authored-By: ValarDragon <ValarDragon@users.noreply.github.com>
* Update mempool/mempool.go
Co-Authored-By: ValarDragon <ValarDragon@users.noreply.github.com>
* Use unknown peer ID
Co-Authored-By: ValarDragon <ValarDragon@users.noreply.github.com>
* fix compilation
* use next wait chan logic when skipping
* Minor fixes
* Add TxInfo
* Add reverse map
* Make activeID's auto-reserve 0
* 0 -> UnknownPeerID
Co-Authored-By: ValarDragon <ValarDragon@users.noreply.github.com>
* Switch to making the normal case set a callback on the reqres object
The recheck case is still done via the global callback, and stats
are also set via global callback
* fix merge conflict
* Addres comments
* Add cache tests
* add cache tests
* minor fixes
* update metrics in reqResCb and reformat code
* goimport -w mempool/reactor.go
* mempool: update memTx senders
I had to introduce txsMap for quick mempoolTx lookups.
* change senders type from []uint16 to sync.Map
Fixes DATA RACE:
```
Read at 0x00c0013fcd3a by goroutine 183:
github.com/tendermint/tendermint/mempool.(*MempoolReactor).broadcastTxRoutine()
/go/src/github.com/tendermint/tendermint/mempool/reactor.go:195 +0x3c7
Previous write at 0x00c0013fcd3a by D[2019-02-27|10:10:49.058] Read PacketMsg switch=3 peer=35bc1e3558c182927b31987eeff3feb3d58a0fc5@127.0.0.1
:46552 conn=MConn{pipe} packet="PacketMsg{30:2B06579D0A143EB78F3D3299DE8213A51D4E11FB05ACE4D6A14F T:1}"
goroutine 190:
github.com/tendermint/tendermint/mempool.(*Mempool).CheckTxWithInfo()
/go/src/github.com/tendermint/tendermint/mempool/mempool.go:387 +0xdc1
github.com/tendermint/tendermint/mempool.(*MempoolReactor).Receive()
/go/src/github.com/tendermint/tendermint/mempool/reactor.go:134 +0xb04
github.com/tendermint/tendermint/p2p.createMConnection.func1()
/go/src/github.com/tendermint/tendermint/p2p/peer.go:374 +0x25b
github.com/tendermint/tendermint/p2p/conn.(*MConnection).recvRoutine()
/go/src/github.com/tendermint/tendermint/p2p/conn/connection.go:599 +0xcce
Goroutine 183 (running) created at:
D[2019-02-27|10:10:49.058] Send switch=2 peer=1efafad5443abeea4b7a8155218e4369525d987e@127.0.0.1:46193 channel=48 conn=MConn{pipe} m
sgBytes=2B06579D0A146194480ADAE00C2836ED7125FEE65C1D9DD51049
github.com/tendermint/tendermint/mempool.(*MempoolReactor).AddPeer()
/go/src/github.com/tendermint/tendermint/mempool/reactor.go:105 +0x1b1
github.com/tendermint/tendermint/p2p.(*Switch).startInitPeer()
/go/src/github.com/tendermint/tendermint/p2p/switch.go:683 +0x13b
github.com/tendermint/tendermint/p2p.(*Switch).addPeer()
/go/src/github.com/tendermint/tendermint/p2p/switch.go:650 +0x585
github.com/tendermint/tendermint/p2p.(*Switch).addPeerWithConnection()
/go/src/github.com/tendermint/tendermint/p2p/test_util.go:145 +0x939
github.com/tendermint/tendermint/p2p.Connect2Switches.func2()
/go/src/github.com/tendermint/tendermint/p2p/test_util.go:109 +0x50
I[2019-02-27|10:10:49.058] Added good transaction validator=0 tx=43B4D1F0F03460BD262835C4AA560DB860CFBBE85BD02386D83DAC38C67B3AD7 res="&{CheckTx:gas_w
anted:1 }" height=0 total=375
Goroutine 190 (running) created at:
github.com/tendermint/tendermint/p2p/conn.(*MConnection).OnStart()
/go/src/github.com/tendermint/tendermint/p2p/conn/connection.go:210 +0x313
github.com/tendermint/tendermint/libs/common.(*BaseService).Start()
/go/src/github.com/tendermint/tendermint/libs/common/service.go:139 +0x4df
github.com/tendermint/tendermint/p2p.(*peer).OnStart()
/go/src/github.com/tendermint/tendermint/p2p/peer.go:179 +0x56
github.com/tendermint/tendermint/libs/common.(*BaseService).Start()
/go/src/github.com/tendermint/tendermint/libs/common/service.go:139 +0x4df
github.com/tendermint/tendermint/p2p.(*peer).Start()
<autogenerated>:1 +0x43
github.com/tendermint/tendermint/p2p.(*Switch).startInitPeer()
```
* explain the choice of a map DS for senders
* extract ids pool/mapper to a separate struct
* fix literal copies lock value from senders: sync.Map contains sync.Mutex
* use sync.Map#LoadOrStore instead of Load
* fixes after Ismail's review
* rename resCbNormal to resCbFirstTime
Marko
Anton Kaliaev
ninjaahhh
Pau
Callum
Tess Rinearson
Erik Grinaker
Phil Salant
Jon
Jun Kimura
Thane Thomson
Ethan Buchman
HaoyangLiu
Dev Ojha