Skip to content
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

Simplify fee charging #1516

Merged

Conversation

tinker-michaelj
Copy link
Collaborator

@tinker-michaelj tinker-michaelj commented Jun 2, 2021

Related issue(s):
Closes #1461

Summary of the change:

  • Replace the overly complicated ItemizableFeeCharging with a NarratedFeeCharging whose methods support exactly the charging actions that Services actually takes.
  • Add some EETs using UncheckedSubmit to verify charging actions taken in case of a node not performing due diligence.
    • Add uncheckedSubmit=2-50 to dev/api-permission.properties to allow this.
  • Extract a PolicyChargingAgent from AwareProcessLogic to encapsulate the choice of which charging actions to take.

External impacts:
None.

Applicable documentation

  • Javadoc

tinker-michaelj added 24 commits May 22, 2021 10:51
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Signed-off-by: tinker-michaelj <[email protected]>
Comment on lines 29 to 30
import com.hedera.services.txns.submission.SystemPrecheck;
import com.hedera.services.utils.SignedTxnAccessor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two imports can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Copy link
Contributor

@qnswirlds qnswirlds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the charging policy needs to be revised. Please see inline comments.

Signed-off-by: tinker-michaelj <[email protected]>
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to resolve newly introduced sonar issues(if any)

It compares to master...I addressed the issues and will check if it passes now...

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #1516 (ab8449c) into master-plus-sdk-0.15.0-alpha.1 (aaee84b) will increase coverage by 0.04%.
The diff coverage is 90.44%.

Impacted file tree graph

@@                         Coverage Diff                          @@
##             master-plus-sdk-0.15.0-alpha.1    #1516      +/-   ##
====================================================================
+ Coverage                             91.27%   91.32%   +0.04%     
+ Complexity                             5876     5848      -28     
====================================================================
  Files                                   461      461              
  Lines                                 16897    16852      -45     
  Branches                               1781     1778       -3     
====================================================================
- Hits                                  15423    15390      -33     
+ Misses                                 1009      999      -10     
+ Partials                                465      463       -2     
Impacted Files Coverage Δ
...om/hedera/services/context/TransactionContext.java 100.00% <ø> (ø)
...era/services/ledger/accounts/BackingTokenRels.java 96.96% <ø> (-0.18%) ⬇️
...services/ledger/accounts/FCMapBackingAccounts.java 96.42% <ø> (ø)
...vices/legacy/services/state/AwareProcessLogic.java 34.69% <0.00%> (+4.33%) ⬆️
...ces/state/exports/SignedStateBalancesExporter.java 96.70% <ø> (-0.04%) ⬇️
...com/hedera/services/utils/PlatformTxnAccessor.java 100.00% <ø> (ø)
...in/java/com/hedera/services/utils/TxnAccessor.java 0.00% <ø> (ø)
...services/fees/charging/NarratedLedgerCharging.java 85.33% <85.33%> (ø)
...a/com/hedera/services/context/ServicesContext.java 93.73% <88.88%> (+0.26%) ⬆️
...dera/services/context/AwareTransactionContext.java 100.00% <100.00%> (+0.82%) ⬆️
... and 11 more

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 aaee84b...ab8449c. Read the comment docs.

void chargePayerAllFees();
void chargePayerServiceFee();
void chargePayerNetworkAndUpToNodeFee();
void chargeSubmittingNodeUpToNetworkFee();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this function will charge the submitting node up to the network fee as a penalty for not performing basic due diligence or for submitting a duplicate transaction with another node? I know some of the function names have enough details, but some of them may still need javadoc to explain.

Copy link
Contributor

@qnswirlds qnswirlds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than maybe adding some javadoc.

tinker-michaelj and others added 2 commits June 2, 2021 10:54
if (payerExempt) {
return;
}
ledger.adjustBalance(grpcNodeId, +nodeFee);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the + could be removed for adjustBalance.

Comment on lines +111 to +113
if (!narratedCharging.isPayerWillingToCoverNetworkFee()) {
narratedCharging.chargeSubmittingNodeUpToNetworkFee();
return INSUFFICIENT_TX_FEE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the logic here is that if the payer offers a large amount of fee willing to pay, but it actually has almost zero balance, in this cause, the submitting node will be charged and the payer walked away with nothing to lose?

Copy link
Collaborator Author

@tinker-michaelj tinker-michaelj Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node should check that the payer will be able to afford the network fee before it submits the txn...if it doesn't check, it is penalized for this lack of "due diligence".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If it does and the transaction goes through, the submitting node will get reward as compensation.

@tinker-michaelj tinker-michaelj merged commit b9b794a into master-plus-sdk-0.15.0-alpha.1 Jun 2, 2021
@tinker-michaelj tinker-michaelj deleted the 01461-M0150-SimplifyFeeCharging branch June 2, 2021 16:37
Copy link
Contributor

@ljianghedera ljianghedera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants