From 8717ccf61d2a61497d399ce1958c5d84411099d8 Mon Sep 17 00:00:00 2001 From: Max <82761650+MaxMustermann2@users.noreply.github.com> Date: Tue, 19 Dec 2023 15:28:04 +0530 Subject: [PATCH] rpc: fix the `from` address calculation (#4593) * test: add delegation type tests These were skipped intentionally in the previous pull request * rpc: undo all changes to 2023.3.0 * rpc: calculate SenderAddress before `ConvertToEth` `tx.ConvertToEth` silently drops the `ShardID` and `ToShardID` fields. This results in the hash of the transaction changing (either via removal of these fields in the hash calculation or by automatic filling of the values by the node's shard). The different hash calculation results in incorrect sender address calculation. There have been a couple of attempts to fix this issue, which created troubles elsewhere. This pull request reverts to the behaviour seen in 2023.3.0 with a simple edit that calculates the `SenderAddress` before dropping the fields in the call to `ConvertToEth`. --- rpc/blockchain.go | 6 +- rpc/eth/types.go | 80 ++++----------------- rpc/pool.go | 9 ++- rpc/transaction.go | 15 +++- rpc/v2/types.go | 31 ++++---- staking/types/delegation_test.go | 120 +++++++++++++++++++++++++++++++ 6 files changed, 177 insertions(+), 84 deletions(-) diff --git a/rpc/blockchain.go b/rpc/blockchain.go index 284bff5af7..c9a6a13138 100644 --- a/rpc/blockchain.go +++ b/rpc/blockchain.go @@ -464,7 +464,11 @@ func (s *PublicBlockchainService) GetBlockReceipts( r, err = v2.NewReceipt(tx, blockHash, block.NumberU64(), index, rmap[tx.Hash()]) case Eth: if tx, ok := tx.(*types.Transaction); ok { - r, err = v2.NewReceipt(tx, blockHash, block.NumberU64(), index, rmap[tx.Hash()]) + from, err := tx.SenderAddress() + if err != nil { + return nil, err + } + r, err = eth.NewReceipt(from, tx.ConvertToEth(), blockHash, block.NumberU64(), index, rmap[tx.Hash()]) } default: return nil, ErrUnknownRPCVersion diff --git a/rpc/eth/types.go b/rpc/eth/types.go index 2767984cdd..c51d604b2d 100644 --- a/rpc/eth/types.go +++ b/rpc/eth/types.go @@ -74,19 +74,9 @@ type Transaction struct { // representation, with the given location metadata set (if available). // Note that all txs on Harmony are replay protected (post EIP155 epoch). func NewTransaction( - tx *types.EthTransaction, blockHash common.Hash, + from common.Address, tx *types.EthTransaction, blockHash common.Hash, blockNumber uint64, timestamp uint64, index uint64, ) (*Transaction, error) { - from := common.Address{} - var err error - if tx.IsEthCompatible() { - from, err = tx.SenderAddress() - } else { - from, err = tx.ConvertToHmy().SenderAddress() - } - if err != nil { - return nil, err - } v, r, s := tx.RawSignatureValues() result := &Transaction{ @@ -111,59 +101,10 @@ func NewTransaction( return result, nil } -// NewReceipt returns the RPC data for a new receipt. It is unused at the moment. -func NewReceipt(tx *types.EthTransaction, blockHash common.Hash, blockNumber, blockIndex uint64, receipt *types.Receipt) (map[string]interface{}, error) { - senderAddr, err := tx.SenderAddress() - if err != nil { - return nil, err - } - +// NewReceipt returns the RPC data for a new receipt +func NewReceipt(senderAddr common.Address, tx *types.EthTransaction, blockHash common.Hash, blockNumber, blockIndex uint64, receipt *types.Receipt) (map[string]interface{}, error) { ethTxHash := tx.Hash() - for i, _ := range receipt.Logs { - // Override log txHash with receipt's - receipt.Logs[i].TxHash = ethTxHash - } - - fields := map[string]interface{}{ - "blockHash": blockHash, - "blockNumber": hexutil.Uint64(blockNumber), - "transactionHash": ethTxHash, - "transactionIndex": hexutil.Uint64(blockIndex), - "from": senderAddr, - "to": tx.To(), - "gasUsed": hexutil.Uint64(receipt.GasUsed), - "cumulativeGasUsed": hexutil.Uint64(receipt.CumulativeGasUsed), - "contractAddress": nil, - "logs": receipt.Logs, - "logsBloom": receipt.Bloom, - } - - // Assign receipt status or post state. - if len(receipt.PostState) > 0 { - fields["root"] = hexutil.Bytes(receipt.PostState) - } else { - fields["status"] = hexutil.Uint(receipt.Status) - } - if receipt.Logs == nil { - fields["logs"] = [][]*types.Log{} - } - // If the ContractAddress is 20 0x0 bytes, assume it is not a contract creation - if receipt.ContractAddress != (common.Address{}) { - fields["contractAddress"] = receipt.ContractAddress - } - - return fields, nil -} - -// NewReceiptFromTransaction returns the RPC data for a new receipt. It is unused at the moment. -func NewReceiptFromTransaction(tx *types.Transaction, blockHash common.Hash, blockNumber, blockIndex uint64, receipt *types.Receipt) (map[string]interface{}, error) { - senderAddr, err := tx.SenderAddress() - if err != nil { - return nil, err - } - - ethTxHash := tx.Hash() - for i, _ := range receipt.Logs { + for i := range receipt.Logs { // Override log txHash with receipt's receipt.Logs[i].TxHash = ethTxHash } @@ -253,7 +194,11 @@ func blockWithFullTxFromBlock(b *types.Block) (*BlockWithFullTx, error) { } for idx, tx := range b.Transactions() { - fmtTx, err := NewTransaction(tx.ConvertToEth(), b.Hash(), b.NumberU64(), b.Time().Uint64(), uint64(idx)) + from, err := tx.SenderAddress() + if err != nil { + return nil, err + } + fmtTx, err := NewTransaction(from, tx.ConvertToEth(), b.Hash(), b.NumberU64(), b.Time().Uint64(), uint64(idx)) if err != nil { return nil, err } @@ -270,5 +215,10 @@ func NewTransactionFromBlockIndex(b *types.Block, index uint64) (*Transaction, e "tx index %v greater than or equal to number of transactions on block %v", index, b.Hash().String(), ) } - return NewTransaction(txs[index].ConvertToEth(), b.Hash(), b.NumberU64(), b.Time().Uint64(), index) + tx := txs[index].ConvertToEth() + from, err := tx.SenderAddress() + if err != nil { + return nil, err + } + return NewTransaction(from, tx, b.Hash(), b.NumberU64(), b.Time().Uint64(), index) } diff --git a/rpc/pool.go b/rpc/pool.go index c0d4858c2e..ee4e34828f 100644 --- a/rpc/pool.go +++ b/rpc/pool.go @@ -253,7 +253,14 @@ func (s *PublicPoolService) PendingTransactions( continue // Legacy behavior is to not return error here } case Eth: - tx, err = eth.NewTransaction(plainTx.ConvertToEth(), common.Hash{}, 0, 0, 0) + from, err := plainTx.SenderAddress() + if err != nil { + utils.Logger().Debug(). + Err(err). + Msgf("%v error at %v", LogTag, "PendingTransactions") + continue // Legacy behavior is to not return error here + } + tx, err = eth.NewTransaction(from, plainTx.ConvertToEth(), common.Hash{}, 0, 0, 0) if err != nil { utils.Logger().Debug(). Err(err). diff --git a/rpc/transaction.go b/rpc/transaction.go index 53f8f47675..4b45045851 100644 --- a/rpc/transaction.go +++ b/rpc/transaction.go @@ -236,7 +236,13 @@ func (s *PublicTransactionService) newRPCTransaction(tx *types.Transaction, bloc } return NewStructuredResponse(tx) case Eth: - tx, err := eth.NewTransaction(tx.ConvertToEth(), blockHash, blockNumber, timestamp, index) + // calculate SenderAddress before ConvertToEth + senderAddr, err := tx.SenderAddress() + if err != nil { + DoMetricRPCQueryInfo(GetTransactionByHash, FailedNumber) + return nil, err + } + tx, err := eth.NewTransaction(senderAddr, tx.ConvertToEth(), blockHash, blockNumber, timestamp, index) if err != nil { DoMetricRPCQueryInfo(GetTransactionByHash, FailedNumber) return nil, err @@ -763,7 +769,12 @@ func (s *PublicTransactionService) GetTransactionReceipt( return NewStructuredResponse(RPCReceipt) case Eth: if tx != nil { - RPCReceipt, err = eth.NewReceiptFromTransaction(tx, blockHash, blockNumber, index, receipt) + // calculate SenderAddress before ConvertToEth + senderAddr, err := tx.SenderAddress() + if err != nil { + return nil, err + } + RPCReceipt, err = eth.NewReceipt(senderAddr, tx.ConvertToEth(), blockHash, blockNumber, index, receipt) } if err != nil { return nil, err diff --git a/rpc/v2/types.go b/rpc/v2/types.go index 76f36ad40e..c9abbead05 100644 --- a/rpc/v2/types.go +++ b/rpc/v2/types.go @@ -204,20 +204,20 @@ type UndelegateMsg struct { // TxReceipt represents a transaction receipt that will serialize to the RPC representation. type TxReceipt struct { - BlockHash common.Hash `json:"blockHash"` - TransactionHash common.Hash `json:"transactionHash"` - BlockNumber uint64 `json:"blockNumber"` - TransactionIndex uint64 `json:"transactionIndex"` - GasUsed uint64 `json:"gasUsed"` - CumulativeGasUsed uint64 `json:"cumulativeGasUsed"` - ContractAddress *common.Address `json:"contractAddress"` - Logs []*types.Log `json:"logs"` - LogsBloom ethtypes.Bloom `json:"logsBloom"` - ShardID uint32 `json:"shardID"` - From string `json:"from"` - To string `json:"to"` - Root hexutil.Bytes `json:"root"` - Status uint `json:"status"` + BlockHash common.Hash `json:"blockHash"` + TransactionHash common.Hash `json:"transactionHash"` + BlockNumber uint64 `json:"blockNumber"` + TransactionIndex uint64 `json:"transactionIndex"` + GasUsed uint64 `json:"gasUsed"` + CumulativeGasUsed uint64 `json:"cumulativeGasUsed"` + ContractAddress common.Address `json:"contractAddress"` + Logs []*types.Log `json:"logs"` + LogsBloom ethtypes.Bloom `json:"logsBloom"` + ShardID uint32 `json:"shardID"` + From string `json:"from"` + To string `json:"to"` + Root hexutil.Bytes `json:"root"` + Status uint `json:"status"` } // StakingTxReceipt represents a staking transaction receipt that will serialize to the RPC representation. @@ -362,6 +362,7 @@ func NewTxReceipt( sender = senderAddr.String() receiver = "" } else { + // Handle response type for regular transaction sender, err = internal_common.AddressToBech32(senderAddr) if err != nil { return nil, err @@ -403,7 +404,7 @@ func NewTxReceipt( // If the ContractAddress is 20 0x0 bytes, assume it is not a contract creation if receipt.ContractAddress != (common.Address{}) { - txReceipt.ContractAddress = &receipt.ContractAddress + txReceipt.ContractAddress = receipt.ContractAddress } return txReceipt, nil } diff --git a/staking/types/delegation_test.go b/staking/types/delegation_test.go index feadda3cc0..ff0c25c02b 100644 --- a/staking/types/delegation_test.go +++ b/staking/types/delegation_test.go @@ -166,3 +166,123 @@ func TestNoEarlyUnlock(t *testing.T) { t.Errorf("should not allow early unlock") } } + +func TestMaxRateAtLess(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(27) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, true) + if result.Cmp(big.NewInt(0)) != 0 { + t.Errorf("should not allow unlock before 7") + } + finalLength := len(delegation.Undelegations) + if initialLength != finalLength { + t.Errorf("should not remove undelegations before 7") + } +} + +func TestMaxRateAtEqual(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(28) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, true) + if result.Cmp(big.NewInt(4000)) != 0 { + t.Errorf("should withdraw at 7") + } + finalLength := len(delegation.Undelegations) + if initialLength == finalLength { + t.Errorf("should remove undelegations at 7") + } +} + +func TestMaxRateAtExcess(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(29) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, true) + if result.Cmp(big.NewInt(0)) != 0 { + t.Errorf("should not withdraw at 8") + } + finalLength := len(delegation.Undelegations) + if initialLength == finalLength { + t.Errorf("should remove undelegations at 8") + } +} + +func TestNoMaxRateAtLess(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(27) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, false) + if result.Cmp(big.NewInt(0)) != 0 { + t.Errorf("should not allow unlock before 7") + } + finalLength := len(delegation.Undelegations) + if initialLength != finalLength { + t.Errorf("should not remove undelegations before 7") + } +} + +func TestNoMaxRateAtEqual(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(28) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, false) + if result.Cmp(big.NewInt(4000)) != 0 { + t.Errorf("should withdraw at 7") + } + finalLength := len(delegation.Undelegations) + if initialLength == finalLength { + t.Errorf("should remove undelegations at 7") + } +} + +func TestNoMaxRateAtExcess(t *testing.T) { + // recreate it so that all tests can run + delegation := NewDelegation(delegatorAddr, delegationAmt) + lastEpochInCommittee := big.NewInt(1) + curEpoch := big.NewInt(29) + epoch := big.NewInt(21) + amount := big.NewInt(4000) + delegation.Undelegate(epoch, amount) + initialLength := len(delegation.Undelegations) + + result := delegation.RemoveUnlockedUndelegations(curEpoch, lastEpochInCommittee, 7, true, false) + if result.Cmp(big.NewInt(4000)) != 0 { + t.Errorf("should withdraw at 8") + } + finalLength := len(delegation.Undelegations) + if initialLength == finalLength { + t.Errorf("should remove undelegations at 8") + } +}