On Transaction API, add "initially paid fee" and "is refund" #4108

andreibancioiu · 2022-05-24T13:27:09Z

On Transaction API, add:

InitiallyPaidFee (computed wrt. the epoch of the transaction)
IsRefund

…icsData).

codecov-commenter · 2022-05-25T11:02:13Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/rosetta@18ebd42). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 34a6b6f differs from pull request most recent head 71d054c. Consider uploading reports for the commit 71d054c to get more accurate results

@@               Coverage Diff               @@
##             feat/rosetta    #4108   +/-   ##
===============================================
  Coverage                ?   75.30%           
===============================================
  Files                   ?      621           
  Lines                   ?    82490           
  Branches                ?        0           
===============================================
  Hits                    ?    62119           
  Misses                  ?    15671           
  Partials                ?     4700

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18ebd42...71d054c. Read the comment docs.

miiu96 · 2022-05-25T12:31:50Z

go.sum

@@ -33,6 +33,10 @@ github.com/ElrondNetwork/elrond-go-core v1.1.15 h1:CMGtJMsK+fBHsSrh5b3N/okYMywbf
 github.com/ElrondNetwork/elrond-go-core v1.1.15/go.mod h1:Yz8JK5sGBctw7+gU8j2mZHbzQ09Ek4XHJ4Uinq1N6nM=
 github.com/ElrondNetwork/elrond-go-core v1.1.16-0.20220520142226-1f321f3d1726 h1:f8UmoIKktGHrYvjXBquoJiejuzxJtJBQJBtEDe/yftk=
 github.com/ElrondNetwork/elrond-go-core v1.1.16-0.20220520142226-1f321f3d1726/go.mod h1:Yz8JK5sGBctw7+gU8j2mZHbzQ09Ek4XHJ4Uinq1N6nM=
+github.com/ElrondNetwork/elrond-go-core v1.1.16-0.20220523135140-7d93ca7217ce h1:KtcRP5g71Br1rOCvIuKyzjw+6jwqBWSB0ZaA3WuUMlM=


go mod tidy

node/external/timemachine/fee/feeComputer.go

miiu96 · 2022-05-25T12:41:35Z

node/external/transactionAPI/refundDetector.go

+}
+
+// Also see: https://github.com/ElrondNetwork/elastic-indexer-go/blob/master/process/transactions/scrsDataToTransactions.go
+func (detector *refundDetector) isRefund(result refundDetectorInput) bool {


maybe can rename the result variable in something more specific like input or scrInput

Renamed to input.

node/external/timemachine/fee/args.go

node/external/timemachine/fee/feeComputer.go

iulianpascalau · 2022-05-25T17:32:40Z

node/external/timemachine/fee/feeComputer.go

+		return nil, err
+	}
+
+	computer.economicsInstances[epoch] = newInstance


can we get OOMs because of using maps that do not get cleared?
Why not using a cacher instead of the map?

Using a cache is indeed desirable. Currently, our LRU / time-based caches are non-generic. Though we can work around their functionality and use them in this context (e.g. using a string representation of the epoch as a key), do you think it's OK to postpone until upgrading the go version & support generics (added a TODO)?

However, also added a test for analyzing the memory footprint of the fee computer: ~12 MB for 10 000 epochs.

Ok, will keep the TODO for now

node/external/timemachine/fee/feeComputer_test.go

iulianpascalau · 2022-05-25T17:42:00Z

node/external/timemachine/fee/feeComputer_test.go

+	checkComputedFee(t, "50000000000000", computer, 0, 80000, 1000000000, "", nil)
+	checkComputedFee(t, "57500000000000", computer, 0, 80000, 1000000000, "hello", nil)
+	checkComputedFee(t, "57500000000000", computer, 0, 1000000, 1000000000, "hello", contract)
+	// (for review) >>> the following scenarios pass, as well. Is this all right?


The penalize too much gas feature is just to protect against malicious actors that try to "keep for themselves" the whole blockchain by issuing transactions with large values for the gas limit field. These types of transactions will have the fees computed in the same way as any other transactions except that, if a provided fee exceeds 10x the gas consumed when executing, the blockchain will not refund the extra gas provided. The fee is computed the same because of this scenario:
suppose that shard 0 sends a SC call towards shard 1 with a gas limit of 600 million. The fee is completely subtracted from the shard 0 account as that shard does not know what is the actual cost of the call. The tx ends up executing on shard 1 but consumes only 10 million. The resulting SCR towards the shard 0 will have a refund value of 0 because the penalize too much gas feature is on.

node/external/timemachine/fee/feeComputer_test.go

iulianpascalau · 2022-05-25T17:48:29Z

node/external/transactionAPI/apiTransactionProcessor.go

+	initiallyPaidFee, err := atp.feeComputer.ComputeTransactionFee(tx.Tx, int(tx.Epoch))
+	if err != nil {
+		log.Warn("populateInitiallyPaidFee(): unexpected condition, cannot compute fee", "tx", tx.Hash)
+		return


so in this case the tx.InitiallyPaidFee is empty as "no fee"?
The return is no different as of the return on L133. How can we know something went wrong other than looking in the logs?

The error is propagated from this call:
feeComputer.createEconomicsInstance(epoch int) which was kind of tested in the constructor already in order to not hit "late construction errors due to programming errors"
Wondering if we can get rid of the error parameter and ease the handling results as seen in this component

outdated log message? should have been populateComputedFieldInitiallyPaidFee() instead of populateInitiallyPaidFee()

Good catch & simplification.
Fixed to not return an error in ComputeTransactionFee, indeed.

Also fixed the outdated log message.

iulianpascalau · 2022-05-25T17:52:27Z

node/external/transactionAPI/apiTransactionProcessor.go

+
+	isZero := initiallyPaidFee.Cmp(big.NewInt(0)) == 0
+	if isZero {
+		return


why not setting tx.InitiallyPaidFee to "0" and then return? Will be more consistent when consuming the request.

Previously, I wanted to not have the field in the response if that's zero ("0" will not be omitted by the serializer). However, on a second thought, changed to:

func (atp *apiTransactionProcessor) populateComputedFieldInitiallyPaidFee(tx *transaction.ApiTransactionResult) { // Only user-initiated transactions will present an initially paid fee. if tx.Type == string(transaction.TxTypeNormal) || tx.Type == string(transaction.TxTypeInvalid) { fee := atp.feeComputer.ComputeTransactionFee(tx.Tx, int(tx.Epoch)) tx.InitiallyPaidFee = fee.String() } }

@iulianpascalau, @bogdan-rosianu, @miiu96, is this all right?

actually, for 0 it will still return "" because it's a big.Int 🤦‍♂️

Will handle this in next PR 🙏

iulianpascalau · 2022-05-25T17:57:10Z

node/external/transactionAPI/refundDetector.go

+	return &refundDetector{}
+}
+
+// Also see: https://github.com/ElrondNetwork/elastic-indexer-go/blob/master/process/transactions/scrsDataToTransactions.go


wondering how we can remove all these code duplicates

One option would be to create a new repository (and go package), such as transaction-decoder or transaction-classifier, independent of any other package. Then, we would reference this package in elrond-go and elastic-indexer-go.

The newly created repository / package would be quite stable in practice.

E.g. @tanghel created this repository for the JS ecosystem. Similarly, we can create extra, small and stable packages (and with no dependencies) for go.

yup
I think we can easily place this in elrond-go-core since it is a business core-logic function

bogdan-rosianu · 2022-05-26T08:27:57Z

node/external/transactionAPI/apiTransactionProcessor.go

+	initiallyPaidFee, err := atp.feeComputer.ComputeTransactionFee(tx.Tx, int(tx.Epoch))
+	if err != nil {
+		log.Warn("populateInitiallyPaidFee(): unexpected condition, cannot compute fee", "tx", tx.Hash)
+		return


outdated log message? should have been populateComputedFieldInitiallyPaidFee() instead of populateInitiallyPaidFee()

node/external/transactionAPI/refundDetector.go

node/external/transactionAPI/refundDetector_test.go

andreibancioiu added 7 commits May 23, 2022 22:59

On API, populate the field transaction.InitiallyPaidFee (using econom…

b4ec284

…icsData).

Fix for zero fee.

634993b

Sketch "isRefund" detection.

697ba06

Add "isRefund" on SCRs of "GET transaction?withResults=true".

fc2bfea

Fix stub.

4f8e10d

Add some unit tests.

cded0b0

Add unit tests.

f060313

andreibancioiu self-assigned this May 24, 2022

andreibancioiu marked this pull request as ready for review May 24, 2022 13:27

bogdan-rosianu self-requested a review May 24, 2022 13:30

andreibancioiu marked this pull request as draft May 24, 2022 13:41

andreibancioiu added 6 commits May 24, 2022 23:35

Sketch a history-enabled feeComputer.

b3c06df

Fix tests etc.

17bd43d

Fix double-checked locking.

d2d48cc

Refactor / move files.

1a01b11

Add tests & tests objects.

7c47107

Fix tests.

30f7be8

andreibancioiu marked this pull request as ready for review May 25, 2022 10:52

Fix after self-review.

08b2c38

miiu96 reviewed May 25, 2022

View reviewed changes

iulianpascalau self-requested a review May 25, 2022 17:22

iulianpascalau reviewed May 25, 2022

View reviewed changes

bogdan-rosianu reviewed May 26, 2022

View reviewed changes

andreibancioiu added 3 commits May 26, 2022 14:03

Fix after review (part 1).

3efbcc6

Fix after review (part 2).

34a6b6f

Update go.sum.

71d054c

andreibancioiu requested review from iulianpascalau, miiu96 and bogdan-rosianu May 26, 2022 11:50

bogdan-rosianu approved these changes May 26, 2022

View reviewed changes

iulianpascalau approved these changes May 26, 2022

View reviewed changes

miiu96 approved these changes May 27, 2022

View reviewed changes

andreibancioiu merged commit 5e2e01d into feat/rosetta May 27, 2022

andreibancioiu deleted the fee-and-refund branch May 27, 2022 06:38

andreibancioiu added complexity:high complexity:medium area:node-api Node API and removed complexity:high labels Jul 25, 2022

andreibancioiu added type:feature New feature or request interested:rosetta labels Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On Transaction API, add "initially paid fee" and "is refund" #4108

On Transaction API, add "initially paid fee" and "is refund" #4108

andreibancioiu commented May 24, 2022 •

edited

codecov-commenter commented May 25, 2022 •

edited

miiu96 May 25, 2022

andreibancioiu May 26, 2022

miiu96 May 25, 2022

andreibancioiu May 26, 2022

iulianpascalau May 25, 2022

andreibancioiu May 26, 2022 •

edited

iulianpascalau May 26, 2022

iulianpascalau May 25, 2022

iulianpascalau May 25, 2022

bogdan-rosianu May 26, 2022

andreibancioiu May 26, 2022 •

edited

iulianpascalau May 25, 2022

andreibancioiu May 26, 2022

iulianpascalau May 26, 2022

andreibancioiu May 26, 2022

iulianpascalau May 25, 2022

andreibancioiu May 26, 2022 •

edited

iulianpascalau May 26, 2022

bogdan-rosianu May 26, 2022

On Transaction API, add "initially paid fee" and "is refund" #4108

On Transaction API, add "initially paid fee" and "is refund" #4108

Conversation

andreibancioiu commented May 24, 2022 • edited

codecov-commenter commented May 25, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu May 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu May 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu May 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu commented May 24, 2022 •

edited

codecov-commenter commented May 25, 2022 •

edited

andreibancioiu May 26, 2022 •

edited

andreibancioiu May 26, 2022 •

edited

andreibancioiu May 26, 2022 •

edited