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`.

Author: MaxMustermann2

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

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)
 }

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).

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

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
 }

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")
+	}
+}