fix: rpc getblock and getblockstats for blocks with withdrawal transactions (asset unlock) by knst · Pull Request #6336 · dashpay/dash

knst · 2024-10-22T12:25:10Z

Issue being fixed or feature implemented

What was done?

Applied fixes for getblock rpc and getblockstats rpc to make them work with withdrawal transactions (asset unlock).

How Has This Been Tested?

Run updated functional test feature_asset_locks.py without the fix causes a failure:

2024-10-22T12:01:35.902000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/knst/projects/dash-reviews/test/functional/test_framework/test_framework.py", line 160, in main
    self.run_test()
  File "/home/knst/projects/dash-reviews/test/functional/feature_asset_locks.py", line 273, in run_test
    self.test_asset_unlocks(node_wallet, node, pubkey)
  File "/home/knst/projects/dash-reviews/test/functional/feature_asset_locks.py", line 410, in test_asset_unlocks
    self.log.info(f"block info: {node.getblock(block_asset_unlock, 2)}")
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/knst/projects/dash-reviews/test/functional/test_framework/coverage.py", line 49, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/knst/projects/dash-reviews/test/functional/test_framework/authproxy.py", line 148, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Internal bug detected: "MoneyRange(fee)"
core_write.cpp:338 (TxToUniv)
Please report this issue here: https://github.com/dashpay/dash/issues
 (-1)

With patch functional test feature_asset_locks.py succeed.

Breaking Changes

N/A

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

ogabrielides

LGTM

kwvg

ACK 6cd7490

PastaPastaPasta · 2024-10-22T13:41:50Z

src/rpc/blockchain.cpp

+                if (const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(*tx)) {
+                    txfee = opt_assetUnlockTx->getFee();
+                } else {
+                    txfee = 0;
+                }


Suggested change

if (const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(*tx)) {

txfee = opt_assetUnlockTx->getFee();

} else {

txfee = 0;

}

const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(*tx);

CHECK_NONFATAL(opt_assetUnlockTx.has_value());

txfee = opt_assetUnlockTx->getFee();

Taking a step further... How about bc3a811?

Looks reasonable to me!

if (calculate_fee) { CAmount fee = amt_total_in - amt_total_out; if (tx.nType == TRANSACTION_ASSET_UNLOCK) { fee = CHECK_NONFATAL(GetTxPayload<CAssetUnlockPayload>(tx))->getFee(); }

Need to call GetTxPayload twice, I guess over-head is not huge?

That's just reading/deserializing from datastream, no heavy ops. Should be fine imo.

…drawal fee calculation failure

UdjinM6 · 2024-10-22T14:27:13Z

bb6d386cc92a68746ce4ef57555951fffef33eb7:

diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp
index 95487525e1..26604bc4c9 100644
--- a/src/rpc/masternode.cpp
+++ b/src/rpc/masternode.cpp
@@ -407,7 +407,9 @@ static RPCHelpMan masternode_payments()
             if (tx->IsCoinBase()) {
                 continue;
             }
-            if (!tx->IsPlatformTransfer()) {
+            if (tx->IsPlatformTransfer()) {
+                nBlockFees += CHECK_NONFATAL(GetTxPayload<CAssetUnlockPayload>(*tx))->getFee();
+            } else {
                 CAmount nValueIn{0};
                 for (const auto& txin : tx->vin) {
                     uint256 blockHashTmp;
@@ -415,10 +417,6 @@ static RPCHelpMan masternode_payments()
                     nValueIn += txPrev->vout[txin.prevout.n].nValue;
                 }
                 nBlockFees += nValueIn - tx->GetValueOut();
-            } else {
-                if (const auto opt_assetUnlockTx = GetTxPayload<CAssetUnlockPayload>(*tx)) {
-                    nBlockFees += opt_assetUnlockTx->getFee();
-                }
             }
         }

knst · 2024-10-22T14:33:29Z

-            if (!tx->IsPlatformTransfer()) {
+            if (tx->IsPlatformTransfer()) {

why? I submitted fix... though it requires new CI run and re-test that there's no silly bug introduced...

UdjinM6 · 2024-10-22T14:36:59Z

no negative condition + IsPlatformTransfer and platform related fee logic are close to each other

UdjinM6 · 2024-10-22T14:41:11Z

f6169fa is even better 👍

kwvg

ACK f6169fa

src/rpc/masternode.cpp

PastaPastaPasta

utACK b9a46f6

kwvg

utACK b9a46f6

UdjinM6

utACK b9a46f6

ogabrielides

utACK b9a46f6

…th withdrawal transactions (asset unlock) b9a46f6 refactor: use IsPlatformTransfer in core_write and rpc/blockchain (Konstantin Akimov) f6169fa fix: make composite rpc 'masternode payments' to work with withdrawals (Konstantin Akimov) e498378 feat: add test for fee in getmempoolentry (Konstantin Akimov) b0d06f0 feat: add regression test for `getblock` and `getblockstats` for withdrawal fee calculation failure (Konstantin Akimov) ab7172b fix: getblockstats rpc to work with withdrawal transactions (Konstantin Akimov) 96c9b46 fix: getblock for withdrawal transaction if verbosity level is 2 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented dashpay#6335 ## What was done? Applied fixes for `getblock` rpc and `getblockstats` rpc to make them work with withdrawal transactions (asset unlock). ## How Has This Been Tested? Run updated functional test `feature_asset_locks.py` without the fix causes a failure: ``` 2024-10-22T12:01:35.902000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/home/knst/projects/dash-reviews/test/functional/test_framework/test_framework.py", line 160, in main self.run_test() File "/home/knst/projects/dash-reviews/test/functional/feature_asset_locks.py", line 273, in run_test self.test_asset_unlocks(node_wallet, node, pubkey) File "/home/knst/projects/dash-reviews/test/functional/feature_asset_locks.py", line 410, in test_asset_unlocks self.log.info(f"block info: {node.getblock(block_asset_unlock, 2)}") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/knst/projects/dash-reviews/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/knst/projects/dash-reviews/test/functional/test_framework/authproxy.py", line 148, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: Internal bug detected: "MoneyRange(fee)" core_write.cpp:338 (TxToUniv) Please report this issue here: https://github.com/dashpay/dash/issues (-1) ``` With patch functional test `feature_asset_locks.py` succeed. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK b9a46f6 kwvg: utACK b9a46f6 UdjinM6: utACK b9a46f6 ogabrielides: utACK dashpay@b9a46f6 Tree-SHA512: e49cf73bff5fabc9463ae538c6c556d02b3f9e396e0353f5ea0661afa015259409cdada406d05b77bf0414761c76a013cd428ffc283cbdefbefe3384c9d6ccc5

d627a6e chore: bump version to 21.1.1 (pasta) 5f9700c docs: release notes for v21.1.1 (pasta) 1c00726 Merge #6277: chore: add builder key for kittywhiskers (pasta) a2bc0f1 Merge #6290: chore: update pasta gpg key to reflect new subkeys (pasta) 167608c Merge #6338: ci: attest results of guix builds (pasta) 6fb4e49 Merge #6197: ci: always build guix, save artifacts (pasta) c0ca93c Merge #6340: fix: make 6336 compile in v21.1.x branch, using older CHECK_NONFATAL functionality (pasta) bb96df4 Merge #6336: fix: rpc getblock and getblockstats for blocks with withdrawal transactions (asset unlock) (pasta) 8e70262 Merge #6131: feat: make a support of Qt app to show Platform transfer Tx (pasta) 80ed279 Merge #6328: backport: bitcoin#30131, bitcoin#23258, bitcoin#30504 - fix bild for Ubuntu 24.10 + clang (pasta) bd772fb Merge #6229: fix: `creditOutputs` in AssetLock tx json output should be an array of objects, not debug strings (pasta) 9bf39a9 Merge #6222: fix: adjust payee predictions after mn_rr activation, add tests (pasta) 87bebfc Merge #6219: fix: correct is_snapshot_cs in VerifyDB (pasta) a4e6b8a Merge #6208: fix: persist coinjoin denoms options from gui over restarts (pasta) Pull request description: ## Issue being fixed or feature implemented See commits, backports, release notes, version bump ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK d627a6e kwvg: ACK d627a6e UdjinM6: utACK d627a6e ogabrielides: utACK d627a6e Tree-SHA512: cde7e40760e16e9f48da8149c3742d18a34029b057405e4d55b87110da96acbcd19b47280451dd7b5ad1ccfc91fde655452cf5f0f0d1e01a41b4c685337c64b8

knst added the backport-candidate-21.1.x label Oct 22, 2024

knst added this to the 21.2 milestone Oct 22, 2024

knst requested review from PastaPastaPasta, UdjinM6, kwvg and ogabrielides October 22, 2024 12:25

knst mentioned this pull request Oct 22, 2024

Internal bug detected: 'MoneyRange(fee)' #6335

Closed

ogabrielides previously approved these changes Oct 22, 2024

View reviewed changes

kwvg previously approved these changes Oct 22, 2024

View reviewed changes

PastaPastaPasta reviewed Oct 22, 2024

View reviewed changes

knst added 4 commits October 22, 2024 21:11

fix: getblock for withdrawal transaction if verbosity level is 2

96c9b46

fix: getblockstats rpc to work with withdrawal transactions

ab7172b

feat: add regression test for getblock and getblockstats for with…

b0d06f0

…drawal fee calculation failure

feat: add test for fee in getmempoolentry

e498378

knst dismissed ogabrielides’s stale review via bb6d386 October 22, 2024 14:13

knst force-pushed the fix-getblock branch from 6cd7490 to bb6d386 Compare October 22, 2024 14:13

knst dismissed kwvg’s stale review via bb6d386 October 22, 2024 14:21

knst force-pushed the fix-getblock branch from bb6d386 to 2bb8d2f Compare October 22, 2024 14:29

fix: make composite rpc 'masternode payments' to work with withdrawals

f6169fa

knst force-pushed the fix-getblock branch from 2bb8d2f to f6169fa Compare October 22, 2024 14:39

kwvg previously approved these changes Oct 22, 2024

View reviewed changes

knst requested a review from PastaPastaPasta October 22, 2024 14:43

PastaPastaPasta reviewed Oct 22, 2024

View reviewed changes

src/rpc/masternode.cpp Show resolved Hide resolved

refactor: use IsPlatformTransfer in core_write and rpc/blockchain

b9a46f6

knst dismissed kwvg’s stale review via b9a46f6 October 22, 2024 14:48

PastaPastaPasta approved these changes Oct 22, 2024

View reviewed changes

kwvg approved these changes Oct 22, 2024

View reviewed changes

UdjinM6 approved these changes Oct 22, 2024

View reviewed changes

ogabrielides approved these changes Oct 22, 2024

View reviewed changes

PastaPastaPasta merged commit 51ecd2e into dashpay:develop Oct 22, 2024

knst deleted the fix-getblock branch October 22, 2024 15:34

UdjinM6 modified the milestones: 21.2, 21.1.1 Oct 22, 2024

UdjinM6 removed the backport-candidate-21.1.x label Oct 22, 2024

knst mentioned this pull request Oct 31, 2024

fix: devnet stuck on block 2 with crash #6375

Merged

5 tasks

Conversation

knst commented Oct 22, 2024

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

knst Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Oct 22, 2024

Uh oh!

knst commented Oct 22, 2024

Uh oh!

UdjinM6 commented Oct 22, 2024

Uh oh!

UdjinM6 commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

UdjinM6 commented Oct 22, 2024 •

edited

Loading