New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[txpool] fix #4215, support allowed transaction list. #4218
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
28c16e4
[txpool] fix #4215, support allowed transaction list.
peekpi bf64234
test: fix node tests for allowlist of txs
MaxMustermann2 0c98cab
Merge pull request #1 from MaxMustermann2/allowedtxs
peekpi 1bc29b7
add error checking of tx.data
peekpi e33aa93
Merge branch 'allowedtxs' of https://github.com/peekpi/harmony into a…
peekpi c24a05c
recover test scripts
peekpi d52a963
config migration
peekpi 915944e
add tx.data to error info
peekpi bcc5efd
[pool] refactor: log more if `from` in denylist
MaxMustermann2 c48d618
reject tx if it does not pass the allowed whiltelist check
peekpi File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package main | ||
|
||
import ( | ||
"bytes" | ||
"testing" | ||
|
||
"github.com/ethereum/go-ethereum/common" | ||
ethCommon "github.com/ethereum/go-ethereum/common" | ||
"github.com/harmony-one/harmony/core" | ||
) | ||
|
||
func TestAllowedTxsParse(t *testing.T) { | ||
testData := []byte(` | ||
0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f->0x855Ac656956AF761439f4a451c872E812E3900a4:0x | ||
0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f->one1np293efrmv74xyjcz0kk3sn53x0fm745f2hsuc:0xa9059cbb | ||
one1s4dvv454dtmkzsulffz3epewsyhrjq9y0g3fqz->0x985458E523dB3d53125813eD68c274899e9DfAb4:0xa9059cbb | ||
one1s4dvv454dtmkzsulffz3epewsyhrjq9y0g3fqz->one10fhdp2g9q5azrs2ukk608x6krd4rleg0ueskug:0x | ||
`) | ||
expected := map[ethCommon.Address]core.AllowedTxData{ | ||
common.HexToAddress("0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f"): core.AllowedTxData{ | ||
To: common.HexToAddress("0x855Ac656956AF761439f4a451c872E812E3900a4"), | ||
Data: common.FromHex("0x"), | ||
}, | ||
common.HexToAddress("0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f"): core.AllowedTxData{ | ||
To: common.HexToAddress("0x985458E523dB3d53125813eD68c274899e9DfAb4"), | ||
Data: common.FromHex("0xa9059cbb"), | ||
}, | ||
common.HexToAddress("0x855Ac656956AF761439f4a451c872E812E3900a4"): core.AllowedTxData{ | ||
To: common.HexToAddress("0x985458E523dB3d53125813eD68c274899e9DfAb4"), | ||
Data: common.FromHex("0xa9059cbb"), | ||
}, | ||
common.HexToAddress("0x855Ac656956AF761439f4a451c872E812E3900a4"): core.AllowedTxData{ | ||
To: common.HexToAddress("0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f"), | ||
Data: common.FromHex("0x"), | ||
}, | ||
} | ||
got, err := parseAllowedTxs(testData) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if len(got) != len(expected) { | ||
t.Errorf("lenght of allowed transactions not equal, got: %d expected: %d", len(got), len(expected)) | ||
} | ||
for from, txData := range got { | ||
expectedTxData := expected[from] | ||
if expectedTxData.To != txData.To || !bytes.Equal(expectedTxData.Data, txData.Data) { | ||
t.Errorf("txData not equal: got: %v expected: %v", txData, expectedTxData) | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
package core | ||
|
||
import ( | ||
"bytes" | ||
"fmt" | ||
"math" | ||
"math/big" | ||
|
@@ -95,10 +96,12 @@ var ( | |
ErrInvalidMsgForStakingDirective = errors.New("staking message does not match directive message") | ||
|
||
// ErrBlacklistFrom is returned if a transaction's from/source address is blacklisted | ||
ErrBlacklistFrom = errors.New("`from` address of transaction in blacklist") | ||
ErrBlacklistFrom = errors.New("`from` address of transaction in blacklist and not in allowlist") | ||
|
||
// ErrBlacklistTo is returned if a transaction's to/destination address is blacklisted | ||
ErrBlacklistTo = errors.New("`to` address of transaction in blacklist") | ||
|
||
ErrAllowedTxs = errors.New("transaction allowed whitelist check failed.") | ||
) | ||
|
||
var ( | ||
|
@@ -145,6 +148,11 @@ type blockChain interface { | |
SubscribeChainHeadEvent(ch chan<- ChainHeadEvent) event.Subscription | ||
} | ||
|
||
type AllowedTxData struct { | ||
To common.Address | ||
Data []byte | ||
} | ||
|
||
// TxPoolConfig are the configuration parameters of the transaction pool. | ||
type TxPoolConfig struct { | ||
Locals []common.Address // Addresses that should be treated by default as local | ||
|
@@ -162,7 +170,8 @@ type TxPoolConfig struct { | |
|
||
Lifetime time.Duration // Maximum amount of time non-executable transaction are queued | ||
|
||
Blacklist map[common.Address]struct{} // Set of accounts that cannot be a part of any transaction | ||
Blacklist map[common.Address]struct{} // Set of accounts that cannot be a part of any transaction | ||
AllowedTxs map[common.Address]AllowedTxData // Set of allowed transactions can break the blocklist | ||
} | ||
|
||
// DefaultTxPoolConfig contains the default configurations for the transaction | ||
|
@@ -181,7 +190,8 @@ var DefaultTxPoolConfig = TxPoolConfig{ | |
|
||
Lifetime: 30 * time.Minute, | ||
|
||
Blacklist: map[common.Address]struct{}{}, | ||
Blacklist: map[common.Address]struct{}{}, | ||
AllowedTxs: map[common.Address]AllowedTxData{}, | ||
} | ||
|
||
// sanitize checks the provided user configurations and changes anything that's | ||
|
@@ -213,6 +223,10 @@ func (config *TxPoolConfig) sanitize() TxPoolConfig { | |
utils.Logger().Warn().Msg("Sanitizing nil blacklist set") | ||
conf.Blacklist = DefaultTxPoolConfig.Blacklist | ||
} | ||
if conf.AllowedTxs == nil { | ||
utils.Logger().Warn().Msg("Sanitizing nil allowedTxs set") | ||
conf.AllowedTxs = DefaultTxPoolConfig.AllowedTxs | ||
} | ||
if conf.AccountSlots == 0 { | ||
utils.Logger().Warn(). | ||
Uint64("provided", conf.AccountSlots). | ||
|
@@ -707,20 +721,33 @@ func (pool *TxPool) validateTx(tx types.PoolTransaction, local bool) error { | |
} | ||
return ErrInvalidSender | ||
} | ||
// Make sure transaction does not have blacklisted addresses | ||
if _, exists := (pool.config.Blacklist)[from]; exists { | ||
if b32, err := hmyCommon.AddressToBech32(from); err == nil { | ||
return errors.WithMessagef(ErrBlacklistFrom, "transaction sender is %s", b32) | ||
|
||
// do whitelist check first, if tx not in whitelist, do blacklist check | ||
if allowedTx, exists := pool.config.AllowedTxs[from]; exists { | ||
if to := tx.To(); to == nil || *to != allowedTx.To || !bytes.Equal(tx.Data(), allowedTx.Data) { | ||
toAddr := common.Address{} | ||
if to != nil { | ||
toAddr = *to | ||
} | ||
return errors.WithMessagef(ErrAllowedTxs, "transaction sender: %x, receiver: %x, input: %x", tx.From(), toAddr, tx.Data()) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Can you change the error string of |
||
return ErrBlacklistFrom | ||
} | ||
// Make sure transaction does not burn funds by sending funds to blacklisted address | ||
if tx.To() != nil { | ||
if _, exists := (pool.config.Blacklist)[*tx.To()]; exists { | ||
if b32, err := hmyCommon.AddressToBech32(*tx.To()); err == nil { | ||
return errors.WithMessagef(ErrBlacklistTo, "transaction receiver is %s", b32) | ||
} else { | ||
// do blacklist check | ||
// Make sure transaction does not have blacklisted addresses | ||
if _, exists := (pool.config.Blacklist)[from]; exists { | ||
if b32, err := hmyCommon.AddressToBech32(from); err == nil { | ||
return errors.WithMessagef(ErrBlacklistFrom, "transaction sender is %s", b32) | ||
} | ||
return ErrBlacklistFrom | ||
} | ||
// Make sure transaction does not burn funds by sending funds to blacklisted address | ||
if tx.To() != nil { | ||
if _, exists := (pool.config.Blacklist)[*tx.To()]; exists { | ||
if b32, err := hmyCommon.AddressToBech32(*tx.To()); err == nil { | ||
return errors.WithMessagef(ErrBlacklistTo, "transaction receiver is %s with data: %x", b32, tx.Data()) | ||
} | ||
return ErrBlacklistTo | ||
} | ||
return ErrBlacklistTo | ||
} | ||
} | ||
// Drop non-local transactions under our own minimal accepted gas price | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
data
needed? Is the combination offrom
andto
not enough?Assuming
data
is needed, do we need any error checking for thedataStr
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the data for smart contract call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think data is necessary, this feature is only for saving tokens from compromised users. All they should do is to send the token out to a new address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the ERC20 token transfer needs to call a smart contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without data, users can only save ONE but ERC20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right. It is better to add error checking