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

Enforce OP_RETURN standardness #1726

Closed
wants to merge 1 commit into from
Closed

Enforce OP_RETURN standardness #1726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/wallet/src/wallet/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ pub enum CreateTxError {
MissingNonWitnessUtxo(OutPoint),
/// Miniscript PSBT error
MiniscriptPsbt(MiniscriptPsbtError),
/// Multiple recipients are OP_RETURN outputs
MultipleOpReturn,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we disallowing multiple OP_RETURN outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the same reason as the byte limitation, it would be non-standard

Copy link
Member

Choose a reason for hiding this comment

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

Fair, I didn't know this was a policy. I think it's best to document this and include a 🔗 to the code enforcing the policy.

https://github.com/bitcoin/bitcoin/blob/3867d2421ae45c9093ceb355f5373c09e6f93c5d/src/policy/policy.cpp#L162

/// The OP_RETURN data contains too many bytes
MaxDataCarrierSize,
}

impl fmt::Display for CreateTxError {
Expand Down Expand Up @@ -171,6 +175,12 @@ impl fmt::Display for CreateTxError {
CreateTxError::MiniscriptPsbt(err) => {
write!(f, "Miniscript PSBT error: {}", err)
}
CreateTxError::MultipleOpReturn => {
write!(f, "Multiple recipients are OP_RETURN outputs")
}
CreateTxError::MaxDataCarrierSize => {
write!(f, "The OP_RETURN data contains too many bytes")
}
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions crates/wallet/src/wallet/mod.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Although OP_RETURN outputs larger than 83 are non-standard, they are still considered to be valid transactions. I think we should still allow building non-standard txs in build_tx.

I think the better approach would be to have TxBuilder::add_data return an error. This provides instant-feedback for the caller and we can document the restriction on the method.

Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ pub use persisted::*;
pub use utils::IsDust;

const COINBASE_MATURITY: u32 = 100;
const MAX_OP_RETURN_BYTES: usize = 83;

/// A Bitcoin wallet
///
Expand Down Expand Up @@ -1388,12 +1389,23 @@ impl Wallet {
let mut received = Amount::ZERO;

let recipients = params.recipients.iter().map(|(r, v)| (r, *v));
let mut contains_op_return = false;

for (index, (script_pubkey, value)) in recipients.enumerate() {
if !params.allow_dust && value.is_dust(script_pubkey) && !script_pubkey.is_op_return() {
return Err(CreateTxError::OutputBelowDustLimit(index));
}

if script_pubkey.is_op_return() {
if contains_op_return {
return Err(CreateTxError::MultipleOpReturn);
}
if script_pubkey.len() > MAX_OP_RETURN_BYTES {
return Err(CreateTxError::MaxDataCarrierSize);
}
contains_op_return = true;
}
rustaceanrob marked this conversation as resolved.
Show resolved Hide resolved

if self.is_mine(script_pubkey.clone()) {
received += value;
}
Expand Down
Loading