Skip to content

Conversation

@rustaceanrob
Copy link
Contributor

Closes bitcoindevkit/bdk_wallet#44

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@ValuedMammal
Copy link
Collaborator

Should we use a default max size of 83 as shown here?

https://github.com/bitcoin/bitcoin/blob/110183746150428e6385880c79f8c5733b1361ba/src/policy/policy.h#L68-L72

@notmandatory notmandatory added audit Suggested as result of external code audit module-wallet labels Nov 18, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 18, 2024
@notmandatory notmandatory added the api A breaking API change label Nov 19, 2024
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK f7d6dcf

@notmandatory notmandatory removed this from the 1.0.0-beta milestone Nov 20, 2024
/// 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

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.

@rustaceanrob
Copy link
Contributor Author

I think this is best put off for 2.0.0. The functionality to build OP_RETURN transactions is present and this is really a matter of standardness, which is up for debate

@rustaceanrob
Copy link
Contributor Author

This will likely be resolved with a new tx_builder crate by 2.0.0 and wouldn't be worth the rebase. Closing

@rustaceanrob rustaceanrob deleted the op-return-11-16 branch December 18, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change audit Suggested as result of external code audit module-wallet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Enforce standardness rules for TxBuilder::add_data

5 participants