Skip to content

Move and rename TxOut default trait to a const called NULL#1838

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
yancyribbens:tx-out-null
May 9, 2023
Merged

Move and rename TxOut default trait to a const called NULL#1838
apoelstra merged 1 commit intorust-bitcoin:masterfrom
yancyribbens:tx-out-null

Conversation

@yancyribbens
Copy link
Copy Markdown
Contributor

@yancyribbens yancyribbens commented May 5, 2023 •
edited
Loading

Create an associated constant const TxOut::NULL for consensus signing code and remove the default trait. Note I tried to deprecate the default() fn instead of just removing it but it doesn't seem to be possible. Also because TxOut::NULL is const, ScriptBuf::new() needed to be changed to const fn.

@yancyribbens yancyribbens mentioned this pull request May 5, 2023
Merged
@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented May 5, 2023

This change is a followup from #1811

@yancyribbens yancyribbens force-pushed the tx-out-null branch 2 times, most recently from 0530c83 to 933f060 Compare May 5, 2023 09:16
@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented May 5, 2023

Address sanitizer test fail seems unrelated

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 5, 2023

Yeah, asan failure looks like it's some nighly-related thing.

You are correct that there is no way to deprecate a Default impl :(. I suggest that we just delete it and put a warning in the CHANGELOG.

apoelstra
apoelstra previously approved these changes May 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 933f06014e4bd55924a327d6cc8b16763c023871

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented May 5, 2023

Updated CHANGELOG.md

tcharding
tcharding previously approved these changes May 5, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK cff5c302cdf8ad56971110a65e867f8e320aaa37

@yancyribbens yancyribbens force-pushed the tx-out-null branch 2 times, most recently from 02ea7c3 to f33445a Compare May 6, 2023 08:09
apoelstra
apoelstra previously approved these changes May 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK f33445a7d930a97157d85c1ebf45cb51dd53ac89

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented May 7, 2023

Note that if we change Amount to be bounded by MAX_MONEY this will have to go away. However, this is still an improvement so I won't block this.

Kixunil
Kixunil requested changes May 7, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Other than incorrect changelog, this is good.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 7, 2023

Note that if we change Amount to be bounded by MAX_MONEY this will have to go away. However, this is still an improvement so I won't block this.

So, it basically can't "go away" since it's used in the legacy sighashing algorithm. We'd have to think carefully about that.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented May 7, 2023

it basically can't "go away" since it's used in the legacy sighashing algorithm

It can by refactoring that single piece of code that uses it to serialize directly into hasher rather than creating intermediate transaction - which would also improve speed so it's nice to do regardless.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented May 7, 2023

Ok, yeah, I agree with that. Though I also think this PR is a step in the right direction.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 8, 2023 •
edited
Loading

We sometimes use an "unreleased" section at the top of the changlog, you can always add that if you'd like to make sure your PRs get a mention, saves me having to decide if something is mention-worthy also which is great :)

@tcharding tcharding requested a review from apoelstra May 8, 2023 00:27
@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 8, 2023

I re-requested review to remove your "approve" @apoelstra since I was momentarily confused by the approve but the changlog thing stlil there.

tcharding
tcharding previously approved these changes May 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

With changelog fix: ACK f33445a7d930a97157d85c1ebf45cb51dd53ac89

@yancyribbens yancyribbens dismissed stale reviews from tcharding and apoelstra via 75b3f19 May 8, 2023 07:01
@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented May 8, 2023 •
edited
Loading

A different version of nightly is now available 1.71 07-05 which is causing a test failure for nightly now.

@yancyribbens yancyribbens mentioned this pull request May 8, 2023
Merged
apoelstra
apoelstra approved these changes May 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 75b3f19

sanket1729 added a commit that referenced this pull request May 9, 2023
@sanket1729
Merge #1841: docs: Add doc comments for external crates
7c6a7c3 
639c548 docs: Add doc comments for external crates (yancy)

Pull request description:

  A new version of rust nightly is causing a test failure for external crates that are not documented.  This is currently causing a failure on my branch here: #1838

  As a side note, it feels like this should be locked to a specific version of nightly in the CI tests and then updated as a choir instead of having it create a new problem in a branch that's not related.

ACKs for top commit:
  apoelstra:
    ACK 639c548
  tcharding:
    ACK 639c548

Tree-SHA512: 764999edc448bff88013301bef287b16ac750af560931470257f2b4475747d52546e73d7fd9a7bdf2cedb18178f370150351e40f176f3da929edf686f02f5c1a
Kixunil
Kixunil approved these changes May 9, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 75b3f19

@apoelstra apoelstra merged commit d93e781 into rust-bitcoin:master May 9, 2023
@stevenroose
Copy link
Copy Markdown
Collaborator

stevenroose commented May 10, 2023

ACK :)

@apoelstra apoelstra mentioned this pull request May 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yancyribbens @apoelstra @Kixunil @tcharding @stevenroose