only broadcast valid transactions #2308
Conversation
rlan35
requested review from
fxfactorial,
gupadhyaya,
Daniel-VDM and
rhazberries
|
@rlan35 , Please cherry-pick this change to s3 as well. |
| } | ||
|
|
||
| // Add new staking transactions to the pending staking transaction list. | ||
| func (node *Node) addPendingStakingTransactions(newStakingTxs staking.StakingTransactions) { | ||
| func (node *Node) addPendingStakingTransactions(newStakingTxs staking.StakingTransactions) []error { |
There was a problem hiding this comment.
why aren't we not broadcasting txns here?
There was a problem hiding this comment.
well I guess addPendingStakingTransactions() is only used from addPendingStakingTransaction, so no need. nvm. checked the code.
| errs := node.addPendingStakingTransactions(staking.StakingTransactions{newStakingTx}) | ||
| for i := range errs { | ||
| if errs[i] != nil { | ||
| return |
There was a problem hiding this comment.
if there is at least one error, we don't broadcast any txns?
There was a problem hiding this comment.
ah nevermind. was confused because was a slice of errors, but its just one transaction.
Better to use if errs[0] != nil { return }
| } | ||
|
|
||
| // AddPendingTransaction adds one new transaction to the pending transaction list. | ||
| // This is only called from SDK. | ||
| func (node *Node) AddPendingTransaction(newTx *types.Transaction) { | ||
| if newTx.ShardID() == node.NodeConfig.ShardID { | ||
| node.addPendingTransactions(types.Transactions{newTx}) | ||
| errs := node.addPendingTransactions(types.Transactions{newTx}) | ||
| for i := range errs { |
There was a problem hiding this comment.
ditto Better to use if errs[0] != nil { return }
| return | ||
| } | ||
| } | ||
| utils.Logger().Info().Str("Hash", newTx.Hash().Hex()).Msg("Broadcasting Tx") |
There was a problem hiding this comment.
we should put this log line above before early error return so that log also prints for error case.
| if node.NodeConfig.ShardID == shard.BeaconChainShardID && | ||
| node.Blockchain().Config().IsPreStaking(node.Blockchain().CurrentHeader().Epoch()) { | ||
| poolTxs := types.PoolTransactions{} | ||
| for _, tx := range newStakingTxs { | ||
| poolTxs = append(poolTxs, tx) | ||
| } | ||
| node.TxPool.AddRemotes(poolTxs) | ||
| errs := node.TxPool.AddRemotes(poolTxs) | ||
| pendingCount, queueCount := node.TxPool.Stats() | ||
| utils.Logger().Info(). | ||
| Int("length of newStakingTxs", len(poolTxs)). | ||
| Int("totalPending", pendingCount). | ||
| Int("totalQueued", queueCount). |
There was a problem hiding this comment.
nit: add err txns counts for log print
|
@rlan35 good catch. My comments are all nits. No need to update as a followup. can just take care of them on other related commits. |
Previously we blindly re-broadcast all received txns from RPC no matter it's valid or spammer txns. This pr correct that to only rebroadcast the txn if the current node think it's valid (not being a aid to spammers)
Tested locally, it shielded the spamming effect from other nodes.