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.

Sign up for GitHub

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

Jump to bottom

feat: new rpc getrawtransactionmulti #5839

Merged
merged 3 commits into from
Jan 23, 2024
Merged

feat: new rpc getrawtransactionmulti #5839

merged 3 commits into from
Jan 23, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jan 22, 2024 •
edited
Loading

Issue being fixed or feature implemented

For platform needs getrawtransactionmulti will help to reduce amount of rpc calls for sake of performance improvement.

What was done?

Implemented new RPC, basic functional test, release note.

How Has This Been Tested?

On testnet:

> getrawtransactionmulti '{"000000abbe61a4d9b9356cb1d7deb1132d0b444a62869e71c2f3aa8ce2361359":["6e3ef19a3f955ac75a1f84dae60d42bbe11548ef54e37033ff2d91b3c4a09e9c", "415d5fafd5ee24ada8b99c36df339785a3066170c0dca6bb1aa6a5b96cf51e35"], "0":["ec7090f01c0e9b6e29d3be8810b12c780d2fb34372a53b231ce18bb7d2f1e8b0"]}'
> getrawtransactionmulti '{"000000abbe61a4d9b9356cb1d7deb1132d0b444a62869e71c2f3aa8ce2361359":["6e3ef19a3f955ac75a1f84dae60d42bbe11548ef54e37033ff2d91b3c4a09e9c", "415d5fafd5ee24ada8b99c36df339785a3066170c0dca6bb1aa6a5b96cf51e35"], "0":["ec7090f01c0e9b6e29d3be8810b12c780d2fb34372a53b231ce18bb7d2f1e8b0"]}'  true

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

@knst knst added this to the 20.1 milestone Jan 22, 2024
@DashCoreAutoGuix
Copy link

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

@DashCoreAutoGuix
Copy link

Guix Automation has began to build this PR tagged as v20.1.0-devpr5839.e58c48c4. A new comment will be made when the image is pushed.

@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Jan 22, 2024
@DashCoreAutoGuix
Copy link

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5839.e58c48c4. The image should be on dockerhub soon.

UdjinM6
UdjinM6 requested changes Jan 22, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

pls see below + needs tests and maybe some release notes

src/rpc/rawtransaction.cpp Outdated
@@ -278,7 +354,7 @@ static UniValue gettxchainlocks(const JSONRPCRequest& request)
},
},
RPCResult{
RPCResult::Type::ARR, "", "Response is an array with the same size as the input txids",
RPCResult::Type::OBJ_DYN, "", "Response is an array with the same size as the input txids",
Copy link

Choose a reason for hiding this comment

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

?

src/txmempool.cpp Outdated
Comment on lines 560 to 563
bool CTxMemPool::getSpentIndex(CSpentIndexKey &key, CSpentIndexValue &value) const
{
LOCK(cs);
mapSpentIndex::iterator it;

it = mapSpent.find(key);
mapSpentIndex::const_iterator it = mapSpent.find(key);
Copy link

Choose a reason for hiding this comment

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

?

src/txmempool.h Outdated
@@ -609,7 +609,7 @@ class CTxMemPool
bool removeAddressIndex(const uint256 txhash);

void addSpentIndex(const CTxMemPoolEntry &entry, const CCoinsViewCache &view);
bool getSpentIndex(CSpentIndexKey &key, CSpentIndexValue &value);
bool getSpentIndex(CSpentIndexKey &key, CSpentIndexValue &value) const;
Copy link

Choose a reason for hiding this comment

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

?

src/rpc/rawtransaction.cpp Outdated
continue;
}

for (const auto idx : irange::range(txids.size())) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
for (const auto idx : irange::range(txids.size())) {
for (const auto idx : irange::range(txids.size())) {

src/rpc/rawtransaction.cpp Outdated
Comment on lines 315 to 316
CBlockIndex* blockindex{blockhash.IsNull() ? nullptr : WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(blockhash))};
if (!blockindex) {
Copy link

Choose a reason for hiding this comment

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

A block hash of 0 indicates transactions not yet mined or in the mempool.

not sure if this code works correctly for this ^^^ case, needs tests

@knst knst removed the guix-build label Jan 22, 2024
@knst knst force-pushed the getrawtransactionmulti branch from e58c48c to 491d246 Compare January 22, 2024 19:45
@DashCoreAutoGuix
Copy link

Guix Automation has began to build this PR tagged as v20.1.0-devpr5839.491d246d. A new comment will be made when the image is pushed.

@knst knst removed the guix-build label Jan 22, 2024
@knst
Copy link
Collaborator Author

knst commented Jan 22, 2024

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5839.e58c48c4. The image should be on dockerhub soon.

previous version had some bugs. Verbose mode didn't worked as expected, didn't worked as expected case of mempool ("0")

Please, wait this build over to use it:

Guix Automation has began to build this PR tagged as v20.1.0-devpr5839.491d246d. A new comment will be made when the image is pushed.

@DashCoreAutoGuix
Copy link

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5839.491d246d. The image should be on dockerhub soon.

UdjinM6
UdjinM6 approved these changes Jan 22, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

PastaPastaPasta
PastaPastaPasta approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for squash merge; the release notes are a bit bare-boned but it's ok

@PastaPastaPasta PastaPastaPasta merged commit d176653 into dashpay:develop Jan 23, 2024
10 checks passed
@UdjinM6 UdjinM6 mentioned this pull request Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
20.1
Development

Successfully merging this pull request may close these issues.
5 participants
@knst @DashCoreAutoGuix @UdjinM6 @PastaPastaPasta @thephez