Rename `txid` to `compute_txid` #2366

yancyribbens · 2024-01-20T17:03:49Z

Rename txid to compute_txid and mark txid as deprecated.

coveralls · 2024-01-20T17:04:49Z

Pull Request Test Coverage Report for Build 7630482955

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 84.252%

Totals
Change from base Build 7628173765:	-0.01%
Covered Lines:	19222
Relevant Lines:	22815

💛 - Coveralls

dpc · 2024-01-20T17:18:39Z

bitcoin/src/blockdata/transaction.rs

    }

+    #[deprecated(since = "0.31.0", note = "use compute_txid() instead")]
+    #[doc(alias = "compute_txid")]


I thought we should add alias = "txid" on the compute_txid, so when someone types tx.txid the LSP will suggest compute_txid? But I might be wrong. @Kixunil might know better.

ah oops, yeah that's an easy change. I don't currently use LSP so I don't care either way.

@dpc yes, needs to be the other way around.

dpc · 2024-01-20T17:20:38Z

bitcoin/src/blockdata/transaction.rs

    }

+    #[deprecated(since = "0.31.0", note = "use compute_txid() instead")]
+    #[doc(alias = "compute_txid")]
+    #[doc(hidden)]


doc(hidden) seems unnecessary and might just confuse people. Deprecation message seems like enough. A docstring explaining to use new name seem more helpful.

There's a lint that fails without doc(hidden)

Yes, doc(hidden) is wrong and so is the lint. Which lint is that? Just turn it off and I'll happily complain at the repo of whoever came up with such nonsense..

The lint is defined here: https://github.com/rust-bitcoin/rust-bitcoin/blob/master/bitcoin/src/lib.rs#L36

#![warn(missing_docs)]

I could change the lint level, but I assumed it was put in place for a reason to ensure a good level of documentation. Ideally this lint wouldn't complain if there's a deprecated attribute..

Oh, I didn't notice you didn't write the documentation. You have to add documentation even when it's deprecated. Or maybe especially because people will read it to understand deprecation better, so write a good explanation there.

bitcoin/src/blockdata/transaction.rs

Kixunil · 2024-01-20T18:49:01Z

bitcoin/src/blockdata/transaction.rs

@@ -773,7 +773,7 @@ impl Transaction {
    #[inline]


Why this commit? We have a CI job for this.

Running cargo +nightly fmt before the CI weekly job runs formats all files yet to be formatted by the CI job.

Then just don't do it or only commit relevant changes.

bitcoin/src/blockdata/transaction.rs

Kixunil · 2024-01-20T18:51:47Z

bitcoin/src/blockdata/transaction.rs

    }

+    #[deprecated(since = "0.31.0", note = "use compute_txid() instead")]
+    #[doc(alias = "compute_txid")]


@dpc yes, needs to be the other way around.

Kixunil · 2024-01-20T18:52:50Z

bitcoin/src/blockdata/transaction.rs

    }

+    #[deprecated(since = "0.31.0", note = "use compute_txid() instead")]
+    #[doc(alias = "compute_txid")]
+    #[doc(hidden)]


Yes, doc(hidden) is wrong and so is the lint. Which lint is that? Just turn it off and I'll happily complain at the repo of whoever came up with such nonsense..

Kixunil · 2024-01-20T19:13:41Z

Oh, one more thing: the same applies to wtxid which should be renamed accordingly as well.

yancyribbens · 2024-01-23T13:34:58Z

Renamed ntxid and wtxid as well in this PR.

Computing the txid is computationally expensive, so rename the method accordingly.

Computing the wtxid is computationally expensive, so rename the method accordingly.

Kixunil

One last typo, the rest looks good.

Kixunil · 2024-01-23T16:43:55Z

bitcoin/src/blockdata/transaction.rs

@@ -679,11 +679,21 @@ impl Transaction {
    /// Maximum transaction weight for Bitcoin Core 25.0.
    pub const MAX_STANDARD_WEIGHT: Weight = Weight::from_wu(400_000);

+    /// Computes a "normalized TXID" which does not include any signatures.
+    ///
+    /// This method is deprecated.  Use `ntxid` instead.


Use compute_ntxid

Thanks, good catch.

Computing the ntxid is computationally expensive, so rename the method accordingly.

Kixunil

ACK 7af3a58

apoelstra · 2024-01-23T19:09:14Z

BTW we could arguably drop ntxid entirely. This is a pre-segwit thing that lets you emulate segwit in one important way (identifying transactions independent of witness data, which pre-segwit means "hashing the transaction without the scriptSigs).

On the one hand it's an obscure non-standardized legacy thing. But on the other hand, when you are using pre-segwit transactions, it comes in handy.

In any case, out of scope for this PR.

apoelstra

ACK 7af3a58

Kixunil · 2024-01-23T20:04:25Z

But on the other hand, when you are using pre-segwit transactions, it comes in handy.

Looks useful enough to me.

tcharding

ACK 7af3a58

yancyribbens force-pushed the rename-txid-to-compute-txid branch from b81f8fc to b62d25e Compare January 20, 2024 17:12

dpc reviewed Jan 20, 2024

View reviewed changes

bitcoin/src/blockdata/transaction.rs Outdated Show resolved Hide resolved

Kixunil requested changes Jan 20, 2024

View reviewed changes

Kixunil added the 1.0 Issues and PRs required or helping to stabilize the API label Jan 20, 2024

Kixunil changed the title ~~Rename txid to compute txid~~ Rename txid to compute_txid Jan 22, 2024

Kixunil added the C-bitcoin PRs modifying the bitcoin crate label Jan 22, 2024

yancyribbens force-pushed the rename-txid-to-compute-txid branch from b62d25e to 3c216ff Compare January 23, 2024 11:06

github-actions bot added the C-internals PRs modifying the internals crate label Jan 23, 2024

yancyribbens force-pushed the rename-txid-to-compute-txid branch from 3c216ff to 8068df2 Compare January 23, 2024 14:53

github-actions bot added the ci label Jan 23, 2024

yancyribbens force-pushed the rename-txid-to-compute-txid branch from 8068df2 to cb339a0 Compare January 23, 2024 14:57

yancy added 2 commits January 23, 2024 15:57

Rename txid to compute_txid

57a7613

Computing the txid is computationally expensive, so rename the method accordingly.

Rename wtxid to compute_wtxid

9bbf79b

Computing the wtxid is computationally expensive, so rename the method accordingly.

Kixunil requested changes Jan 23, 2024

View reviewed changes

yancyribbens force-pushed the rename-txid-to-compute-txid branch from cb339a0 to 7af3a58 Compare January 23, 2024 18:53

Rename ntxid to compute_ntxid

7af3a58

Computing the ntxid is computationally expensive, so rename the method accordingly.

Kixunil approved these changes Jan 23, 2024

View reviewed changes

apoelstra approved these changes Jan 23, 2024

View reviewed changes

tcharding approved these changes Jan 24, 2024

View reviewed changes

apoelstra merged commit cf3a7bb into rust-bitcoin:master Jan 24, 2024
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `txid` to `compute_txid` #2366

Rename `txid` to `compute_txid` #2366

yancyribbens commented Jan 20, 2024 •

edited by Kixunil

coveralls commented Jan 20, 2024 •

edited

dpc Jan 20, 2024

yancyribbens Jan 20, 2024

Kixunil Jan 20, 2024

dpc Jan 20, 2024

yancyribbens Jan 20, 2024

Kixunil Jan 20, 2024

yancyribbens Jan 23, 2024

Kixunil Jan 23, 2024

Kixunil Jan 20, 2024

yancyribbens Jan 23, 2024

Kixunil Jan 23, 2024

Kixunil Jan 20, 2024

Kixunil Jan 20, 2024

Kixunil commented Jan 20, 2024

yancyribbens commented Jan 23, 2024

Kixunil left a comment

Kixunil Jan 23, 2024

yancyribbens Jan 23, 2024

Kixunil left a comment

apoelstra commented Jan 23, 2024

apoelstra left a comment

Kixunil commented Jan 23, 2024

tcharding left a comment

Rename txid to compute_txid #2366

Rename txid to compute_txid #2366

Conversation

yancyribbens commented Jan 20, 2024 • edited by Kixunil

coveralls commented Jan 20, 2024 • edited

Pull Request Test Coverage Report for Build 7630482955

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jan 20, 2024

yancyribbens commented Jan 23, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 23, 2024

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Jan 23, 2024

tcharding left a comment

Choose a reason for hiding this comment

Rename `txid` to `compute_txid` #2366

Rename `txid` to `compute_txid` #2366

yancyribbens commented Jan 20, 2024 •

edited by Kixunil

coveralls commented Jan 20, 2024 •

edited