change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response #5994

arya2 · 2023-01-18T05:32:42Z

Motivation

It would be convenient for the getrawmempool output to match zcashd's exactly so we can run zcash-rpc-diff continuously until there's a difference to examine.

Solution

Gets full transactions from mempool
Sorts transactions by fee/size then id before returning getrawmempool results

Review

Anyone can review. This doesn't have to merge, we could just use this branch for #5935.

Reviewer Checklist

Will the PR name make sense to users?
- Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
Are the PR labels correct?
Does the code do what the ticket and PR says?
- Does it change concurrent code, unsafe code, or consensus rules?
How do you know it works? Does it have tests?

Follow Up Work

zebra-rpc/src/methods.rs

teor2345

I think this looks good, but I can't see if the order matches zcashd.

We might also want to make it easier to maintain - zcashd might change their order when they implement ZIP-317.

Can you post some zcash-rpc-diff results that demonstrate fee/size and transaction hash ordering?

zebra-chain/src/transaction/hash.rs

zebra-rpc/src/methods.rs

codecov · 2023-01-18T06:46:00Z

Codecov Report

Merging #5994 (6114a96) into main (67194c4) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5994      +/-   ##
==========================================
+ Coverage   78.01%   78.09%   +0.07%     
==========================================
  Files         312      312              
  Lines       38997    39011      +14     
==========================================
+ Hits        30425    30467      +42     
+ Misses       8572     8544      -28

arya2 · 2023-01-18T18:04:11Z

Can you post some zcash-rpc-diff results that demonstrate fee/size and transaction hash ordering?

I haven't seen it compare transaction hashes yet but for fee/size:

➜  zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952462...

real    0m0.004s
user    0m0.003s
sys     0m0.000s

Querying zcashd main chain at height >=1952462...

real    0m0.004s
user    0m0.004s
sys     0m0.000s


Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.aTB2QNAr5z.rpc-diff/zebrad-main-1952462-getrawmempool.json:
[
  "a6d4cd5bc965d581f2b125da30971993f034b370ffb54bcefaa8af0d452fc1bc",
  "74979ecc884fe5a4f01f78dfe5b8d252fee7b0a11c96492deb61d10024520ece",
  "ab0269e0caf72e467e246d9127affa58b1a28f65bca472d34ded8d1d816cd9e7",
  "36f5a844625e83ef9c01bc3c2a6ce368c22e313717f79784eed9db849a38e41c",
  "49cf002385c1503ddd1255fd5d2a6420f448d7f98dd17d826b5e10d081f917f9"
]

More:

➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952462...

real 0m0.003s
user 0m0.002s
sys 0m0.000s

Querying zcashd main chain at height >=1952462...

real 0m0.005s
user 0m0.000s
sys 0m0.004s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.zp5PHPZImD.rpc-diff/zebrad-main-1952462-getrawmempool.json:
[
"a6d4cd5bc965d581f2b125da30971993f034b370ffb54bcefaa8af0d452fc1bc",
"74979ecc884fe5a4f01f78dfe5b8d252fee7b0a11c96492deb61d10024520ece",
"ab0269e0caf72e467e246d9127affa58b1a28f65bca472d34ded8d1d816cd9e7",
"36f5a844625e83ef9c01bc3c2a6ce368c22e313717f79784eed9db849a38e41c",
"49cf002385c1503ddd1255fd5d2a6420f448d7f98dd17d826b5e10d081f917f9"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952462...

real 0m0.003s
user 0m0.003s
sys 0m0.000s

Querying zcashd main chain at height >=1952462...

real 0m0.004s
user 0m0.003s
sys 0m0.000s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.ufdDnxDee3.rpc-diff/zebrad-main-1952462-getrawmempool.json:
[
"a6d4cd5bc965d581f2b125da30971993f034b370ffb54bcefaa8af0d452fc1bc",
"74979ecc884fe5a4f01f78dfe5b8d252fee7b0a11c96492deb61d10024520ece",
"ab0269e0caf72e467e246d9127affa58b1a28f65bca472d34ded8d1d816cd9e7",
"36f5a844625e83ef9c01bc3c2a6ce368c22e313717f79784eed9db849a38e41c",
"49cf002385c1503ddd1255fd5d2a6420f448d7f98dd17d826b5e10d081f917f9"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.004s
user 0m0.003s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.003s
user 0m0.003s
sys 0m0.000s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.NVA9QwrtE9.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.002s
user 0m0.002s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.002s
user 0m0.002s
sys 0m0.000s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.wVSWLneKAN.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.004s
user 0m0.004s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.004s
user 0m0.004s
sys 0m0.000s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.qyAMIxOqEr.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.004s
user 0m0.003s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.003s
user 0m0.003s
sys 0m0.000s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.WU1AOkIgo4.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.004s
user 0m0.003s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.002s
user 0m0.000s
sys 0m0.002s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.bA4SJYm7xj.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]
➜ zebra git:(sort-getrawmempool) ✗ ZCASH_CLI=../zcash/src/zcash-cli ./zebra-utils/zcash-rpc-diff 3000 getrawmempool
Checking first node release info...
Checking second node release info...
Connected to zebrad (port 3000) and zcashd (../zcash/src/zcash-cli zcash.conf port).

Checking zebrad network and tip height...
Checking zcashd network and tip height...

Request:
getrawmempool

Querying zebrad main chain at height >=1952464...

real 0m0.004s
user 0m0.004s
sys 0m0.000s

Querying zcashd main chain at height >=1952464...

real 0m0.004s
user 0m0.000s
sys 0m0.004s

Response diff between zebrad and zcashd:
RPC responses were identical

/tmp/tmp.lA4ztbtrh9.rpc-diff/zebrad-main-1952464-getrawmempool.json:
[
"18bbe79487e157af34c1b8d00d51cedb3dd07625d8b76b17eb58e9f8c870d11e",
"6077962d04cda6a99502cdfaf71c38079ec4f17186d4e53f94f18b1a39a24ddb",
"bd8881165be076b6e1a342356bc7194f239908e0e8d72c4e75466e6bcefae2b7"
]

- Removes feature flag on get_raw_mempool

zebra-rpc/src/methods.rs

teor2345

There are just a few compile warnings to clear up, then we're good to go.

Change was made

teor2345 · 2023-01-19T02:55:46Z

Also I think there was a temporary network error which made a lot of PRs fail.

Co-authored-by: teor <[email protected]>

teor2345 · 2023-01-19T07:04:18Z

This might need the dependency fix in PR #5992 to build correctly, or some other changes to the dependencies, feature dependencies, or features on the code.

arya2 · 2023-01-19T19:03:37Z

@Mergifyio update

mergify · 2023-01-19T19:04:24Z

update

✅ Branch has been successfully updated

arya2 · 2023-01-19T20:30:19Z

I missed a couple test failures:

---- methods::tests::snapshot::test_rpc_response_data stdout ----

The application panicked (crashed).
Message: condition was false for request: FullTransactions,
in zebra_test::mock_service::MockService<zebra_node_services::mempool::Request, zebra_node_services::mempool::Response, zebra_test::mock_service::PanicAssertion, alloc::boxed::Box<dyn core::error::Error + core::marker::Send + core::marker::Sync>>

https://github.com/ZcashFoundation/zebra/actions/runs/3961606576/jobs/6787274003#step:14:2559

---- methods::tests::prop::mempool_transactions_are_sent_to_caller stdout ----
proptest: Saving this and future failures in /home/runner/work/zebra/zebra/zebra-rpc/proptest-regressions/methods/tests/prop.txt
proptest: If this test was run on a CI system, you may wish to add the following line to your copy of the file. (You may need to create it.)
cc 92c0a529ff6f84a632eb97c92cc7e43a740c6c5bb5ad2338a28cd345cd73bb7b

The application panicked (crashed).
Message: Test failed: assertion failed: (left == right)
left: FullTransactions,
right: TransactionIds: received an unexpected request
in zebra_test::mock_service::MockService<zebra_node_services::mempool::Request, zebra_node_services::mempool::Response, zebra_test::mock_service::PropTestAssertion, alloc::boxed::Box<dyn core::error::Error + core::marker::Send + core::marker::Sync>> at zebra-test/src/mock_service.rs:515; minimal failing input: transaction_ids = {}

https://github.com/ZcashFoundation/zebra/actions/runs/3961606576/jobs/6787274003#step:14:2473

zebra-rpc/src/methods/tests/prop.rs

teor2345

Thanks for fixing this up, let's go!

Sorts transactions like zcashd in getrawmempool

89ebc8b

arya2 added P-Low ❄️ C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces labels Jan 18, 2023

arya2 self-assigned this Jan 18, 2023

arya2 requested a review from a team as a code owner January 18, 2023 05:32

arya2 requested review from dconnolly and removed request for a team January 18, 2023 05:32

github-actions bot added the C-enhancement Category: This is an improvement label Jan 18, 2023

arya2 commented Jan 18, 2023

View reviewed changes

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved

teor2345 previously requested changes Jan 18, 2023

View reviewed changes

zebra-chain/src/transaction/hash.rs Show resolved Hide resolved

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved

arya2 added 3 commits January 18, 2023 17:35

Simplifies sort logic and condenses duplicate code

cd11c81

adds comment clarifying the intended byte order for transaction hashes

25be33d

Multiplies fee by MAX_BLOCK_BYTES/tx-size instead of tx-size

2d7cf50

- Removes feature flag on get_raw_mempool

arya2 requested a review from teor2345 January 19, 2023 02:04

teor2345 reviewed Jan 19, 2023

View reviewed changes

zebra-rpc/src/methods.rs Show resolved Hide resolved

teor2345 reviewed Jan 19, 2023

View reviewed changes

zebra-rpc/src/methods.rs Show resolved Hide resolved

teor2345 reviewed Jan 19, 2023

View reviewed changes

zebra-rpc/src/methods.rs Show resolved Hide resolved

teor2345 reviewed Jan 19, 2023

View reviewed changes

Apply suggestions from code review

c2097bd

Co-authored-by: teor <[email protected]>

Always uses TransactionIds request in tests

78a90af

mergify bot and others added 2 commits January 19, 2023 19:04

Merge branch 'main' into sort-getrawmempool

38c56f8

reverts switch from #[cfg()] to cfg!()

faf7319

arya2 removed the request for review from dconnolly January 19, 2023 20:22

arya2 added 3 commits January 19, 2023 16:27

Adds feature flag to constant

b675f75

Updates tests and removes !cfg(not(test))

fdeb2ef

Moves up comment

0e04938

arya2 requested a review from teor2345 January 19, 2023 22:12

teor2345 changed the title ~~change(rpc): Sorts mempool transactions like zcashd in getrawmempool~~ change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response Jan 19, 2023

teor2345 reviewed Jan 19, 2023

View reviewed changes

zebra-rpc/src/methods/tests/prop.rs Show resolved Hide resolved

arya2 mentioned this pull request Jan 20, 2023

Manually test that the Zebra and zcashd mempools are similar #5935

Closed

3 tasks

adds missing transaction_ids

6114a96

teor2345 approved these changes Jan 23, 2023

View reviewed changes

mergify bot added a commit that referenced this pull request Jan 23, 2023

Merge of #5994

5aad5cd

mergify bot mentioned this pull request Jan 23, 2023

merge queue: embarking main (53b4466) and [#5994 + #6010 + #6014 + #5894 + #6008 + #5968] together #6019

Closed

mergify bot merged commit be2c229 into main Jan 23, 2023

mergify bot deleted the sort-getrawmempool branch January 23, 2023 04:48

oxarbitrage mentioned this pull request Jan 30, 2023

Release v1.0.0-rc.4 #6045

Merged

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response #5994

change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response #5994

arya2 commented Jan 18, 2023 •

edited by teor2345

Loading

teor2345 left a comment

codecov bot commented Jan 18, 2023 •

edited

Loading

arya2 commented Jan 18, 2023

teor2345 left a comment

teor2345 commented Jan 19, 2023

teor2345 commented Jan 19, 2023

arya2 commented Jan 19, 2023

mergify bot commented Jan 19, 2023

arya2 commented Jan 19, 2023

teor2345 left a comment

change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response #5994

change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response #5994

Conversation

arya2 commented Jan 18, 2023 • edited by teor2345 Loading

Motivation

Solution

Review

Reviewer Checklist

Follow Up Work

teor2345 left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 18, 2023 • edited Loading

Codecov Report

arya2 commented Jan 18, 2023

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Jan 19, 2023

teor2345 commented Jan 19, 2023

arya2 commented Jan 19, 2023

mergify bot commented Jan 19, 2023

✅ Branch has been successfully updated

arya2 commented Jan 19, 2023

teor2345 left a comment

Choose a reason for hiding this comment

arya2 commented Jan 18, 2023 •

edited by teor2345

Loading

codecov bot commented Jan 18, 2023 •

edited

Loading