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

Implements createrawtransaction RPC method #9017

Conversation

elijahhampton
Copy link
Contributor

@elijahhampton elijahhampton commented Nov 13, 2024

Implements createrawtransaction RPC method and parameter validation. Tests to be added in follow-up commit.

Motivation

The goal of this PR is to implement the create_raw_transaction RPC endpoint for zebrad. This will support the effort to fully deprecate zcashd and enhance zcashd with all existing zcashd functionality. This PR will close the following issue: #8952

Specifications & References

Solution

  • Adds createrawtransaction RPC implementation
  • Adds parameter validation for inputs.
  • Adds a test suite for createrawtransaction RPC method (Coming soon)

Tests

  • Test will be added in the follow up commit as this is now a draft.

Follow-up Work

  • There will be no follow up work for this pull request once it is completed.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@mpguerra mpguerra linked an issue Nov 14, 2024 that may be closed by this pull request
@elijahhampton elijahhampton marked this pull request as ready for review November 17, 2024 18:52
@elijahhampton elijahhampton requested review from a team as code owners November 17, 2024 18:52
@elijahhampton elijahhampton requested review from upbqdn and removed request for a team November 17, 2024 18:52
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

I left some first view comments but i will stop there for now as i think you are still working here.

I think it is important to have snapshot tests here and it will be good to manually compare results with zcashd (we have a tool to help with that).

@@ -77,6 +77,10 @@ impl Height {
/// height and above.
pub const MAX_EXPIRY_HEIGHT: Height = Height(499_999_999);

/// The number of blocks within expiry height when a tx is considered
/// to be expiring soon .
pub const BLOCK_EXPIRY_HEIGHT_THRESHOLD: u32 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put a link to zcashd where this constant is defined and used ? I want to make sure they are using the same number.

@@ -3,6 +3,8 @@
/// Supported opcodes
///
/// <https://github.com/zcash/zcash/blob/8b16094f6672d8268ff25b2d7bddd6a6207873f7/src/script/script.h#L39>

#[allow(missing_docs)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just add 1 line docs here instead?

/// Returns the hex string of the raw transaction based on the given inputs `transactions`, `addresses`,
/// `locktime` and `expiryheight`.
///
/// zcashd reference: [`z_create_raw_transaction`](https://zcash.github.io/rpc/createrawtransaction.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// zcashd reference: [`z_create_raw_transaction`](https://zcash.github.io/rpc/createrawtransaction.html)
/// zcashd reference: [`createrawtransaction`](https://zcash.github.io/rpc/createrawtransaction.html)

Comment on lines +255 to +256
/// - `addresses`: (string, required) A json object object with addresses as keys and amounts as values.
/// ["address": x.xxx] (numeric, required) They key is the Zcash address, the value is the ZEC amount
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check how other addresses fields are documented for Zebra, for example https://github.com/ZcashFoundation/zebra/blob/main/zebra-rpc/src/methods.rs#L100-L101

/// - `expiryheight`: (numeric, optional, default=nextblockheight+20 (pre-Blossom) or nextblockheight+40 (post-Blossom)) Expiry height of
/// transaction (if Overwinter is active)
///
/// # Notes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// # Notes
/// # Notes
///

let tip_height = best_chain_tip_height(&latest_chain_tip).map_server_error()?;

let lock_time = if let Some(lock_time) = locktime {
LockTime::Height(block::Height(lock_time))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend to get the locktime of the tip here instead ?

));
}

// DoS mitigation: reject transactions expiring soon (as is done in zcashd)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a reference link

}

let tx = match current_upgrade {
NetworkUpgrade::Genesis | NetworkUpgrade::BeforeOverwinter => Transaction::V1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure if we need to support creating transactions that are below V4.

@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Dec 5, 2024
@mpguerra
Copy link
Contributor

Thank you for submitting this PR. Unfortunately it seems like this RPC method won't be implemented in Zebra after all. Sorry about that!

@mpguerra mpguerra closed this Dec 16, 2024
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.

Implement createrawtransaction RPC method
4 participants