dac627cc09 Feature: Psbt fee checks (junderw)
Pull request description:
Closes#2061
These new methods on Psbt will add checks for high fees by default. The threshold for "high fees" is currently set to 25000 sat/vbyte, which is about 20x higher than the highest next block fees seen on the "Mempool" website.
The primary goal of this change is to prevent users of the library from accidentally sending absurd amounts of fees.
(ie. Recently in September 2023 there was a transaction that sent an absurd amount of fees and made news in the Bitcoin world. Luckily the mining pool gave it back, but some might not be so lucky.)
There are variants of the method that allow for users to set their own "absurd" threshold using a `FeeRate` value. And there is a method that performs no checks, and the method name is alarming enough to draw attention in a review, so at least developers will be aware of the concept.
ACKs for top commit:
apoelstra:
ACK dac627cc09
tcharding:
ACK dac627cc09
Tree-SHA512: ae0beafdb50339ba3efc44a48ba19c0aeeb0a2671eb43867c1e02b807677ce99fb6b4c47b74a9ed2999f827b3edc00a8871fa4730dd12a4cb265be99437c13db
c34c709681 CI: add an automated github releases on new tags (Einherjar)
Pull request description:
Creates a `gh-release.yml` that runs on every new tag using the [`ncipollo/release-action`](https://github.com/ncipollo/release-action), which is one of the recommended actions after [GitHub's official `actions/create-release`](https://github.com/actions/create-release) was archived.
The idea is to just have a release with the source code, no binary (although if wanted I can add that as well).
A little bit of context: tcharding told me yesterday at TABConf that he copied manually all commits titles to the release notes somewhere (I think in crates.io?).
This is an easy way to do it, just copy from the GitHub release.
Finally, it is also positive to generate Releases from Tags since this increases the exposure of the repo (GitHub is highly social) while also signaling maturity.
Here's an [example](https://github.com/oven-sh/bun/releases/tag/bun-v0.8.1) that I've found in a non-related repo that has the same `generateReleaseNotes: true` as this PR proposes
<img width="857" alt="image" src="https://github.com/realeinherjar/rust-bitcoin/assets/136860407/41789514-8abf-4f58-b7ca-d2c352346923">
ACKs for top commit:
apoelstra:
ACK c34c709681
tcharding:
ACK c34c709681
Tree-SHA512: 06bc0276b5aca95e19965eadb79d0a4cf8606b222dce37f095247196fcac023458c34e35cf34fa2344bc274063e642b93b05dc796a27179567052c58ac4bc27a
c34e3cc7cc Re-write size/weight API (Tobin C. Harding)
73f7fbf520 Add code comments to transaction serialization (Tobin C. Harding)
29f20c1d0b Add segwit serialization constants (Tobin C. Harding)
Pull request description:
Audit and re-write the weight/size API for `Block` and `Transaction`. First two patches are trivial, patch 3 contains justification and explanation for this work, copied here:
```
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
```
Please note, this PR introduces panics if a sciptPubkey overflows the calculation `weight = spk.size() * 4`.
Fix#2049
ACKs for top commit:
apoelstra:
ACK c34e3cc7cc
sanket1729:
ACK c34e3cc7cc.
Tree-SHA512: 4944f652e6e362a282a5731140a9438a82d243a4c646b4627d9046a9f9cf13c476881750d432cfbc6b5fe5de1f0c4c9c44ed4569dac4bc11b55a5db28793803c
5901d35095 Add push_p2wpkh function on Witness (Tobin C. Harding)
8cd409d561 Deprecate push_bitcoin_signature (Tobin C. Harding)
Pull request description:
In order to create the witness to spend p2wpkh output one must create a `Witness` that includes the signature and the pubkey, we should have a function for this.
## Notes
The PR originally added a `push_p2wphk` method, this is now instead a constrcutor `Witness:p2wpkh` (after review discussion below).
- Patch 1 changes `push_bitcoin_signature` to take an `ecdsa::Sigtnture` instead of an `ecdsa::SerializedSignature`
- Patch 2 takes a `secp256k1::PublicKey` removing the need for an error path (discussed below).
ACKs for top commit:
sanket1729:
ACK 5901d35095
apoelstra:
ACK 5901d35095
Tree-SHA512: 646014d97daafbf0909106d8990debaf481ac6f3578f0ddf232d739c3e2d55ae1d0275abe5a4a1db1c5c192c8c5f0b5546fc65aac37b91a3729db881c5ad3dec
24e5e19ed0 Add changelog entries (Tobin C. Harding)
79f2157311 bitcoin: Grab changelog entry from 0.30.1 release (Tobin C. Harding)
Pull request description:
Gearing up for the next release add changelog entries for everything merged upto July 24.
ACKs for top commit:
apoelstra:
ACK 24e5e19ed0
clarkmoody:
ACK 24e5e19ed0
Tree-SHA512: 12761623f3d61fea77b5dfe4c505dcdb5041ec4473ae05524b818eb2d4e3cbc3b039b8f1400022916c9f2d1ee97fcccd7e7b6da0bb5f868a1b77bdcfade3d7ed
9fa2111c12 Bump model-checking/kani-github-action from 0.36 to 0.37 (dependabot[bot])
Pull request description:
Bumps [model-checking/kani-github-action](https://github.com/model-checking/kani-github-action) from 0.36 to 0.37.
<details>
<summary>Commits</summary>
<ul>
<li><a href="7c99bc77b4"><code>7c99bc7</code></a> Update Kani version to 0.37.0 (<a href="https://redirect.github.com/model-checking/kani-github-action/issues/49">#49</a>)</li>
<li>See full diff in <a href="https://github.com/model-checking/kani-github-action/compare/v0.36...v0.37">compare view</a></li>
</ul>
</details>
<br />
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
</details>
ACKs for top commit:
apoelstra:
ACK 9fa2111c12
clarkmoody:
ACK 9fa2111c12
Tree-SHA512: fb7e2913718aba03915b3ee3d4644ba6acca12012466c1001fe951f35e2877cefd30db83a29c773807829af3ba3ed9f1d2539e19d107509999e507079c43b926
f31fb08651 Remove default impl from transaction version (Tobin C. Harding)
Pull request description:
There is no logical default for the transaction version number, there is only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version they want not rely on us making the choice.
(I originally added this impl to support testing, this was in hindsight the wrong thing to do, props to Sanket for noticing.)
Discussed in comments here: https://github.com/rust-bitcoin/rust-bitcoin/pull/2006#issuecomment-1732385464
ACKs for top commit:
clarkmoody:
ACK f31fb08651
apoelstra:
ACK f31fb08651
Tree-SHA512: 1112c35173d6b2e7d6c50f058b1964fc8b633615ae703b0c1231e701a3e322fd55d3a8377c7ea078f28f523b15b7de6e9eca844226fcd19528206161d75dcfdf
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
The `Witness::push_bitcoin_signature` method is old and a bit stale.
Bitcoin has taproot signatures now so the name is stale, also we have
the `crate::ecdsa::Signature` type that holds the secp sig and the hash
type so we can use that instead of having two separate parameters.
Add a new, up to date, `Witness::push_ecdsa_signature` function and
deprecate the `push_bitcoin_signature` one.
There is no logical default for the transaction version number, there is
only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version
they want not rely on us making the choice.
(I originally added this impl to support testing, this was in hindsight
the wrong thing to do, props to Sanket for noticing.)
One of our stated aims is to make it possible to learn bitcoin by using
our library. To help with this aim add to private consts for the segwit
transaction marker and flag serialization fields.
158ba26a8a Feature: Count sigops for Transaction (junderw)
Pull request description:
I copied over the sigop counting logic from Bitcoin Core, but I made a few adjustments.
1. I removed 2 consensus flags that checked for P2SH and SegWit activation. This code assumes both are activated. If we were to include that, what would be a good way to go about it? (ie. If I run this method on a transaction from the 1000th block and it just so happened to have a P2SH-like input, Bitcoin Core wouldn't accidentally count those sigops because the consensus flag will stop them from running the P2SH logic. Same goes for SegWit)
3. Since there's no guarantee that we have an index from which we can get the prevout scripts, I made it into a generic closure that looks up the prevout script for us. If the caller doesn't provide it, We can only count sigops directly in the scriptSig and scriptPubkey (no P2SH or SegWit).
## TODO
- [x] Write tests for transaction sigop counting
~~Edit: The test changes are just to get the 1.48 tests passing. I'll remove them and replace them with whatever solution that is agreed upon in another PR etc.~~
Edit 2: This is the code I used as a guide:
8105bce5b3/src/consensus/tx_verify.cpp (L147-L166)
Edit 3: I found a subtle bug in the implementation of `count_sigops` (https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687)
ACKs for top commit:
apoelstra:
ACK 158ba26a8a
tcharding:
ACK 158ba26a8a
Tree-SHA512: 2b8a0c50b9390bfb914da1ba687e8599b957c75c511f764a2f3ed3414580150ce3aa2ac7aed97a4f7587d3fbeece269444c65c7449b88f1bdb02e573e6f6febd
e4c7e01a6f Use the new bech32 iterator API (Tobin C. Harding)
Pull request description:
Depend on the newly released version of `bech32`, BOOM!
ACKs for top commit:
apoelstra:
ACK e4c7e01a6f
clarkmoody:
ACK e4c7e01a6f
Tree-SHA512: 91675a830cf67f8dcabd42e7dc1b70d80b669330be5244bb8102e0ec5d1a206d5ead07f73b328a158b761c328bc78d573185af8d31f14183ccc17318d752c02b
95b7a95fc2 test: correct psbt used for regression test (Subhradeep Chakraborty)
Pull request description:
The `unknown` key used in the `Psbt` (in `serde_regression_psbt` test) is not really unknown. Also the `preimages` in the `Input` don't seem to be valid. Though they don't affect the tests because of the `serde::serialize`, the `Psbt` itself is invalid and hence this PR.
ACKs for top commit:
tcharding:
ACK 95b7a95fc2
apoelstra:
ACK 95b7a95fc2
Tree-SHA512: 275320bb273163364640351e55a3f16c34c1eef06e9dde79b72bf40187d313f037edb5e4544f4dce32fb8a7cc552504aa2e4d22a646c89a3dec022f917539219
b2a7d7023c Rename XpubIdentifier to XKeyIdentifier (Tobin C. Harding)
ffd2466ad1 Move XpubIdentifier to the bip32 module (Tobin C. Harding)
Pull request description:
- Patch 1: Move the hash to the `bip32` module where it is used, as we have done with other hashes recently (and re-export it at crate root).
- Patch 2: Rename the hash to `XKeyIdentifier` as discussed in #2014Fix: #2014
ACKs for top commit:
apoelstra:
ACK b2a7d7023c
Tree-SHA512: 5efa9fc857c71e506263bf6adee3b4294f22838d5b119177c9108c69191d545338c11a4796bc95e956a67f3418010725f3d12c06d2c4c3bb5cf038d59976ae0f
71a5fe2b54 Customize Debug implementation of absolute::LockTime (Subhradeep Chakraborty)
Pull request description:
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/2011.
This PR aims to make the "pretty print" of `absolute::LockTime` prettier by printing `X blocks` and `X seconds` for `Blocks` and `Seconds` respectively instead of the default Enum printing.
ACKs for top commit:
apoelstra:
ACK 71a5fe2b54
tcharding:
ACK 71a5fe2b54
Tree-SHA512: 79baad5ee0ba1aa892cb38588dae6a24977b8e42356208d6cc9adb007d1f8371e61dc82e35bb4dac9961216d68c56907f0db376c5c6fbb5a406728b8f7c3f5ad
b229fd5555 Use correct form for noun: carve-out (Tobin C. Harding)
Pull request description:
One "carves out" something, not "carve output" - woops.
ACKs for top commit:
apoelstra:
ACK b229fd5555
Tree-SHA512: 196c5c239da35a67b83eb96f2f9346af6518af51ff6bb4a9e40cddab8c8511f2e913278a9727bfe4e97d50c58cb87a4100e40c13c8313643b04a2d0fabaaa9e5
bc398204bf Remove redundant segwit version from function names (Tobin C. Harding)
Pull request description:
A P2TR output does not need to be clarified with version 1, it is implicit. As with p2wpkh/p2wsh and version 0.
Remove redundant version identifiers from function names, deprecating the originals.
ACKs for top commit:
apoelstra:
ACK bc398204bf
Tree-SHA512: 49806c564badca25ce02161445b2b41497b565f2002aa1edfc0cf0c57b38683480deec0d9b682e18dc7e59c22128e0b641abcccc2cbedd0b5603cbcbf2fd26df
f17bb0d18f Remove unnecessary reference (Tobin C. Harding)
Pull request description:
`T` is a generic that implements`AsRef<PushBytes>`, it should not be a reference. This is inline with other usages of `AsRef<PushBytes>` for example in `Builder::push_slice`.
Found while working on #2003
ACKs for top commit:
apoelstra:
ACK f17bb0d18f
Tree-SHA512: 6f6ae0ba5d5010db53d9c2af107df84bc058277b2b7cc35800f4e6ed93d351838b7f101284b7d80345bee639615d27d76a2e5c4c784782c5b3e5090444defe29
BIP-68 activated a fair while ago (circa 2019) and since then only
transaction versions 1 and 2 have been considered standard.
Currently in our `Transaction` struct we use an `i32`, this means users
can construct a non-standard transaction if they do not first look up
what the value should be. We can help folk out here by abstracting over
the version number.
Since the version number only governs standardness elect to make the
inner `i32` public (ie., not an invariant). The aim of the type is to
make life easy not restrict what versions are used.
Add transaction::Version data type that simply provides two consts `ONE`
and `TWO`.
Add a `Default` impl on `Version` that returns `Version::TWO`.
In tests that used version 0, instead use `Version::default` because the
test obviously does not care.
52f2332383 Remove docs from witness version conversion functions (Tobin C. Harding)
47d6d785cb Remove bip 173/350 test vectors (Tobin C. Harding)
e0eaeaad99 Split ParseError out of Error (Tobin C. Harding)
0f536e86dc Add new UnknownAddressTypeError for parsing address type (Tobin C. Harding)
e2014cba1b Import error variants within dislay impl (Tobin C. Harding)
9d7791fcd6 Remove unnecessary self:: from error import (Tobin C. Harding)
b2e485ed51 Split the address error code out into a separate module (Tobin C. Harding)
f34ca0c52b Move address.rs to address/mod.rs (Tobin C. Harding)
Pull request description:
In preparation for depending on the recently released version of `rust-bech32` do a bunch of preparatory fixes.
1. Improve `address` module error handling as we are doing else where at the moment
2. Remove bip 173 and 350 test vector tests, these are fully covered in bech32
3. Trim down the docs on `WitnessVersion`
This PR is the first 8 patches of https://github.com/rust-bitcoin/rust-bitcoin/pull/1951
ACKs for top commit:
sanket1729:
ACK 52f2332383
apoelstra:
ACK 52f2332383
Tree-SHA512: 67a4ea4020b4e5c9c8396e4195e06dbd1d11335788f9e52f60abbc0b399e37e5dacc9bb7fa4e88221670322fa3c3407ade059d5c709f96e2df97240f4524e08c
ccdcffe69c Re-export Opcode (Tobin C. Harding)
Pull request description:
We recently rename `opcodes::All` to `Opcode` but did not re-export it from the crate root. Since it now has a nice clear name we can do so.
Found while hacking up the `rust-bitcoincore-rpc` dependency upgrade for upcoming release `bitcoin v0.31.0`.
ACKs for top commit:
apoelstra:
ACK ccdcffe69c
Tree-SHA512: 8acac90d01f14245f93edc10f3483d8aa9865aca59b6ef42ab744b875558fca8ad74894ad95a83637f0cec7a2a353d74d4d6f7ee5f1d1276cc0fc3dcb6983362
acbf23aaa5 Add `is_multisig` helper to Script type (Clark Moody)
Pull request description:
A new `is_multisig` helper method to classify bare multisig output scripts.
The form of a valid multisig script is:
- Pushnum `M`
- <N> pubkeys
- Pushnum `N`
- `OP_CHECKMULTISIG`
`N` must equal the number of pushed pubkeys, and `M` must be less than or equal to `N`.
I've tested this against the RPC output of Core at the block level, checking that the total number of multisig outputs matches.
```
Block 350338, 89 multisig
Block 350340, 29 multisig
Block 350341, 4 multisig
Block 350343, 579 multisig
Block 350344, 48 multisig
Block 350346, 11 multisig
Block 350347, 404 multisig
Block 350350, 127 multisig
Block 350351, 1 multisig
Block 350353, 40 multisig
Block 350356, 13 multisig
Block 350357, 2 multisig
Block 350358, 1 multisig
```
ACKs for top commit:
tcharding:
ACK acbf23aaa5
apoelstra:
ACK acbf23aaa5
Tree-SHA512: b8feeaa8725ac63a658897dac3b303fc8b3d56674d796b14569548124928329993bea45482928d9ce85231f1b5837922af8c0a77b2601a92f88b5e2a9394e97f
026a55809e Fix: Script::count_sigops parsing should not return a Result (junderw)
Pull request description:
When implementing some tests for the Transaction PR, I noticed that there were coinbase transactions that would pass Bitcoin Core parsing and fail my code.
It turns out that the Script parsing for sigops calls `break` to exit the loop and returns the current n value whenever there is an EarlyEndOfScript error.
See this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687 for some links to the relevant source.
ACKs for top commit:
apoelstra:
ACK 026a55809e
tcharding:
ACK 026a55809e
Tree-SHA512: 57c1b88add5e1c9ef9245fcec0e471db55c2f9b1b0b0f8ebd471f1bede0ca5eeb8492d8c75dea1fd43f1343037df44969c9b9fde26a7de9ac68a26dca899e47f
cd15c746cb Feature: Instruction can read the script number (junderw)
Pull request description:
This new method on `Instruction` will allow for interpreting the Instruction as a script number (i32)
ACKs for top commit:
tcharding:
ACK cd15c746cb
apoelstra:
ACK cd15c746cb
Tree-SHA512: 597add5ab28b53b7f61633b2e2005bed2c8fe4777575c9498c9a5431e4b020cf8ddf02509b1b4c1ba455e879e024d7d034636ab4863cca42567ed087b648abc3
These docs do not add that much value, we do not typically bother
documenting `From` and `TryFrom` implementations because they are super
well known and its obvious from the function signature what is going on.
The `address::Error` is module level general, we can make the code
easier to maintain and easier to stabalize by splitting the parse error
out of the general error.
Create a `ParseError` that is returned by `FromStr for Address`. Remove
the now unused variants from the general `address::Error`.
In an effort to reduce the number of lines of code import the error
variants locally within the `Display` impl on `Error`.
Refactor only, no logic changes.
Split the error code out of `address/mod.rs` and into
`address/error.rs`. Code move only, no changes other than to
imports/exports etc. to make it build.
S. Santos
Tobin C. Harding
Andrew Poelstra
junderw
dependabot[bot]
Clark Moody
Subhradeep Chakraborty