Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for combining together two PSBTs. Doing so broke the
`psbt` module.
Pull the merge logic out of the `Map` trait and put it in methods on
each individual type (`Input`, `Output`, `PartiallySignedTransaction`).
Doing so allows for simplification of return types since combining
inputs/outputs never errors.
Use the term 'combine' instead of 'merge' since that is the term used in
BIP 174.
006193f5b6 feat: Support running CI locally with `act` (Andrew Ahlers)
Pull request description:
Disable problematic jobs that involve Github Actions caching or `cross` whenever the environment is set to ACT. This allows running the CI pipeline locally and hopefully speeds up PR cycle times by reducing unexpected CI pipeline results.
## Motivation
The CI pipeline does not run until maintainer approval. This allows an easy path for contributors to test out the CI pipeline locally and avoid unexpected results. I personally kept hitting issues here due to MSRV always throwing me off
## Potential issues
- `act` does not support Github Actions caching feature which is used in the `fuzz` workflow so I simply disabled it if `act` is detected.
- the `cross` workflow is similarly disabled. I kept hitting `sh: 1: cargo: not found` (see: https://github.com/cross-rs/cross/issues/260). I tried a few different workaround but had no success.
I'm hoping this is acceptable as it still improves the local testing situation and covers the `Tests` workflow
ACKs for top commit:
apoelstra:
ACK 006193f5b6
Kixunil:
ACK 006193f5b6
Tree-SHA512: 079d50276161ef2c7d61e41d8f47126b8def41ea61f4560be03950e6e4e07ae8bb67ab22ac8f0e46e5e5e3406b64c1de4bb2c62526c36b82db962842ffc377ca
Disable problematic jobs that involve Github Actions caching or `cross`
whenever the environment is set to ACT. This allows running the CI
pipeline locally and hopefully speeds up PR cycle times by reducing
unexpected CI pipeline results.
10fedfb3b4 Change Prevouts::All(&[TxOut]) to Prevouts::All(&[Borrow<T>]) (sanket1729)
Pull request description:
I believe this avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods.
See #834
ACKs for top commit:
apoelstra:
ACK 10fedfb3b4
Kixunil:
ACK 10fedfb3b4
Tree-SHA512: 20f69c626b38d6b3c03c8cb370cfad097bbf0bfefff9bb2379c8af3bc94e25d8cc45fc5d69488aeefad58a95470e8f30eb7b400349992a9ebd0d3a13870cba43
4e19973d4e Add a breaking test (sanket1729)
69c6eb6173 Bug: Change type of pbst partial sig from secp key to bitcoin key (sanket1729)
Pull request description:
This changes the type of secp signature from secp256k1::PublicKey to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
ACKs for top commit:
Kixunil:
ACK 4e19973d4e
apoelstra:
ACK 4e19973d4e
Tree-SHA512: 2a326bafc050cd101e75899c32224a9ac2fcb2ec0b9f7f173404a46f2b3a92ecb78d0002db252a5af06705566bdc10102d20f4718eaeeebd3730fdb5ee89ff5a
This avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods. Facilitates creation of Prevouts from &[TxOut] as well as
&[&TxOut]
This changes the type of secp signature from secp256k1::Signature to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
79cee4cd31 fix: Error on unsuported addresses in `is_signed_by_address` (Andrew Ahlers)
Pull request description:
This is a direct response to this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/684#issuecomment-1012136845
Currently unsupported addresses simply return `false` which is highly misleading as it is entirely possible that the keys related to a given address were used to sign the message.
If this is successful I can follow up with
> a method to check if a PublicKey is associated with an address
ACKs for top commit:
apoelstra:
ACK 79cee4cd31
Kixunil:
ACK 79cee4cd31
Tree-SHA512: 0c2f15696c63272e8ad1ecc0959c828f2d6c4aa16a7d099235c3f6bd287a0c20e034752331644c4bcc3af61ba4d739ad6795cbcea61c9e615c1eb7b43ebf0eeb
df7bb03a67 Simplify read_scriptbool (Tobin Harding)
4b6e86658d Refactor is_provably_unspendable (Tobin Harding)
e54a2d653b Put && operator at front of line (Tobin Harding)
f5512c4931 Refactor is_p2pkh (Tobin Harding)
373ea89a9a Simplify read_scriptbool (Tobin Harding)
654b2772b8 Add passing unit tests for read_scriptbool (Tobin Harding)
Pull request description:
In an effort to make the code clearer and more explicit, do various refactorings around logical operators. Each done as a separate patch to ease review and limit scope of discussion.
Based on review of https://github.com/rust-bitcoin/rust-bitcoin/pull/806
ACKs for top commit:
Kixunil:
ACK df7bb03a67
apoelstra:
ACK df7bb03a67
Tree-SHA512: 06460979d492eb38cefc147397338b7fd95320c66ce8e8b4f8e2b454bb35721ce308413690a0618bd19d695df56175646d4d0c619388c0268f7fd35d5a7b6a3d
Simplify `read_scriptbool` by doing:
- Use `split_last` to get at the last element
- Mask the last byte against ^0x80 instead of using two equality
statements
22aeaef52b Use write_all instead of write (Riccardo Casatta)
Pull request description:
write() could write only a part of the given buffer, the caller should
check the numbers of byte written (which is what write_all does)
ACKs for top commit:
apoelstra:
ACK 22aeaef52b
Kixunil:
ACK 22aeaef52b
dr-orlovsky:
utACK 22aeaef52b
Tree-SHA512: e4bf3c757e1d369f9bb737e970b93ec29a487419eb478b41c36da033eafea3f0a96faa1e5c6f9397febba309ce4330b29a9e5369bb547645e5f72ba935d2cafe
08428ba32b Add a disabled rustfmt.toml (Tobin Harding)
Pull request description:
We do not currently use `rustfmt`, this is a nuisance for devs who routinely work on code bases that do use rustfmt who often have their editors set to format on save. We can make the life of such devs much better by explicitly disabling formatting using `rustfmt.toml`.
ref: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting
ACKs for top commit:
thomaseizinger:
tACK 08428ba32b
Kixunil:
ACK 08428ba32b
sanket1729:
ACK 08428ba32b
apoelstra:
ACK 08428ba32b
Tree-SHA512: f8a87206c9b484a308d1dbb754b33c89563ada98184eecc8e2d24cfe9425018d4a5457b15233058d92df07374c29a226af855100e6b273a06a35208416675e5d
151173821b Use fn name to_ instead of into_ (Tobin Harding)
Pull request description:
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `into_` is for owned to owned
non-`Copy` types.
Re-name conversion methods that use `into_` for `Copy` types to use
`to_`, no need to deprecate these ones because they are unreleased.
**Note to maintainers**
This is similar in concept to #798 but only touches new code introduced in this release. Has been labelled 'RC fix' for that reason. Please feel free to remove the label if you disagree.
From the docs: https://rust-lang.github.io/api-guidelines/naming.html
<h2><a class="header" href="https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv" id="ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv">Ad-hoc conversions follow <code>as_</code>, <code>to_</code>, <code>into_</code> conventions (C-CONV)</a></h2>
<p>Conversions should be provided as methods, with names prefixed as follows:</p>
Prefix | Cost | Ownership
-- | -- | --
as_ | Free | borrowed -> borrowed
to_ | Expensive | borrowed -> borrowed
| | | borrowed -> owned (non-Copy types)
| | | owned -> owned (Copy types)
into_ | Variable | owned -> owned (non-Copy types)
ACKs for top commit:
Kixunil:
ACK 151173821b
apoelstra:
ACK 151173821b
sanket1729:
ACK 151173821b
Tree-SHA512: 4bb97e4fb78beda0fd1ec9482d24ef0f4ade6d3689f5c1bcf2208fa2df3195962522fa5d5ac700e6d4d5ff2096a20b2a0ad51784909a3c12405762aa08d1ced2
We do not currently use `rustfmt`, this is a nuisance for devs who
routinely work on code bases that do use rustfmt who often have their
editors set to format on save. We can make the life of such devs much
better by explicitly disabling formatting using `rustfmt.toml`.
ref: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting
42e10ff132 Only use two newlines before level 2 headings (Tobin Harding)
9be2fb6088 Remove Sebastian from maintainers list (Tobin Harding)
Pull request description:
@sgeisler requested by email to be step down from the position of
maintainer of `rust-bitcoin`. Remove his name from the contributing document's list of maintainers.
Implement @dr-orlovsky's suggested [whitespace policy ](https://github.com/rust-bitcoin/rust-bitcoin/pull/795#discussion_r786513512)while we are at it.
ACKs for top commit:
Kixunil:
ACK 42e10ff132
dr-orlovsky:
ACK 42e10ff132
apoelstra:
utACK 42e10ff132
Tree-SHA512: f2e3f3bde5b2e42b0144328dbe4c5c1471764cd5cf0270c71aeee68c4cf962bfc2fd1a5718d4db3809113e5f5a6b92ce0c76582c6d8426a216b025b726fb0e54
8d780e8172 Use 4 character indentation (Tobin Harding)
Pull request description:
Bizarrely the impl block for `EcdsaSigHashType` uses 5 character indentation.
Use 4 character indentation as is typical.
ACKs for top commit:
Kixunil:
ACK 8d780e8172
apoelstra:
ACK 8d780e8172
Tree-SHA512: 4b0789b4ef6f3e817fcbe002ca0d9ee4fca4a3dc70bbeaebae745a2f0497eb072f3c3876805d97ff58eb8a9464ea4443f23cea0c95bfd052e382caa9f76b515d
b138428df7 Re-export public map types from root level (Tobin Harding)
Pull request description:
We currently have the `map` module private but containing a bunch of types that are needed in the public API (specifically in a `PartiallySignedTransaction`).
To give access to them re-export the `util::psbt` module at the root level.
Found while testing `master` with `rust-miniscript`.
ACKs for top commit:
sanket1729:
utACK b138428df7
Kixunil:
ACK b138428df7
RCasatta:
ACK b138428df7
dr-orlovsky:
ACK b138428df7
Tree-SHA512: 36fc8595164c4975abdadb6c8149ef27686a2d681a1815379f91b1bd36f8a56ceaa7faed5979ba6869823684790721a16a0c41e662c6227a09cd0ba576a0a181
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `into_` is for owned to owned
non-`Copy` types.
Re-name conversion methods that use `into_` for `Copy` types to use
`to_`, no need to deprecate these ones because they are unreleased.
9407d103c5 Be uniform in heading spacing (Tobin Harding)
af9389d724 Add Kixunil, RCassata, and tcharding to maintainers (Tobin Harding)
Pull request description:
We recently added `CONTRIBUTING.md` but forgot to add @Kixunil . Also a couple days ago I was added to the repo.
Patch two is trivial cleanup of whitespace.
ACKs for top commit:
sanket1729:
utACK 9407d103c5. Did not verify emails :)
Kixunil:
ACK 9407d103c5
RCasatta:
ACK 9407d103c5
elichai:
ACK 9407d103c5
apoelstra:
ACK 9407d103c5
Tree-SHA512: 032e847972a418a425f91364b8850a398f814406dc109f216cc783feefc7db9504e75c2f7f2d4b4d42b2bb44e7d4ef8c5641a7741389ce7ab2f9063d12afcc88
1b2dbd7c08 0.28.0-rc.1 release (Dr Maxim Orlovsky)
Pull request description:
We still have quite a few issues and PRs pending to be addressed/merged before full 0.28 release: see https://github.com/rust-bitcoin/rust-bitcoin/milestone/10
Most important, we have to find a way to address #777; additionally it will be desirable to get #587, #786, #776.
We also have quite of work to write CHANGELOG in #785
Nevertheless I propose to start with a `beta-1` subrelease to unlock ability for `miniscript` release and downstream testing. The Taproot API looks final, and the pending PRs are addresses internal issues/bugs which is perfectly fine for beta releases.
ACKs for top commit:
Kixunil:
ACK 1b2dbd7c08
Tree-SHA512: a7bd6dd3e7a489f7fd758fb0d7a60103bfe0cdf88997b2163f163841fa1c12e7b31fa8f03b9f817a671379578dbc10a2ca583f49b64d2d2de4a9ebb57b2b9bd5
We currently have the `map` module private but containing a bunch of
types that are needed in the public API (specifically in a
`PartiallySignedTransaction`).
Re-export the publicly required types to the `psbt` module and then
again at the root level of `rust-bitcoin` as we do for other types.
We currently sometimes have one newline before headings and sometimes
two, its not important which it is but uniformity is nice.
Use two newlines before headings uniformly in `CONTRIBUTING.md`.
31c49832b7 Fix IRC log record on gnusha.org (Dr Maxim Orlovsky)
e1c8e13cb8 Contributing: improving language and style (Dr Maxim Orlovsky)
45dbaa7e26 Contributing: remove derivation section (Dr Maxim Orlovsky)
313ac7d4a8 Contributing: improve formatting section (Dr Maxim Orlovsky)
78d1a82769 Contributing guidelines (Dr Maxim Orlovsky)
Pull request description:
We've being discussing that a `CONTRIBUTING.md` guidelines are needed for some time and I took an effort to propose it. The main text is taken from `rust-lightning` version (with some additions), but it is extended on the topics we were discussing in multiple issues previously:
- list of repository maintainers
- standard derives - #555
- naming conventions (after Bitcoin Core)
- MSRV and 2018 update #510
- code formatting with `rustfmt` #172
Each of these issue-related amendments to the basic (modified) version of rust-lightning contributing guidelines are made with a separate commit.
ACKs for top commit:
dr-orlovsky:
Sorry for disturbing, @RCasatta @sanket1729, but this needs just a review of a single fixup commit in 31c49832b7 since your last reviews of this PR. We don't want to merge without your ACKs since you did a lot of commenting here before.
RCasatta:
ACK 31c4983
Kixunil:
ACK 31c49832b7
apoelstra:
ACK 31c49832b7
sanket1729:
utACK 31c49832b7. Looks good
Tree-SHA512: 1b15334a4e867070c0b33ba5fbdb55aea90d09879254673cf93c8bea069118b3543289f6f291e085bf2dd90715b913b783bcb9b025b9113079095cf14c220275
abe52f681b Cleanup/Dedup psbt (De)Serialization code (sanket1729)
fbd86dcf63 Update documentation of EcdsaSig::from_slice (sanket1729)
85009a7b50 Update documentation of from_u32_consensus (sanket1729)
0fed04e2d5 Change EcdsaSig hash type deser (sanket1729)
Pull request description:
Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value
ACKs for top commit:
dr-orlovsky:
ACK abe52f681b
apoelstra:
ACK abe52f681b
Kixunil:
ACK abe52f681b
Tree-SHA512: 1d5dbe3aa5885ca16649cf8ea05a7476e8dd977dd870b79358d97a3ce383bee93754d2b88163e7db3792cdc4b9cb867356409c8eea4e110877577ad196ba0786
dfd8924398 Remove insert_pair from Map trait (Tobin Harding)
ad75d5181f Make Map trait private to psbt module (Tobin Harding)
53225c0a6e Improve docs in map module (Tobin Harding)
92059c2841 Add full stops to rustdocs (Tobin Harding)
11c046b707 Refactor match arms (Tobin Harding)
e6af569490 Move imports to top of file (Tobin Harding)
Pull request description:
The `Map` method `insert_pair` is never called for `PartiallySignedTransaction`. Separate the method into its own trait (`Insert`) and delete dead code. The dead code contains the alleged bug in #576.
- Patch 1: Preparatory cleanup
- Patch 2: Preparatory refactor
- Patch 3 and 4: Improve docs in the module that this PR touches
- Patch 5: Make `Map` trait private to the `psbt` module
- ~Patch 6: Make `concensus_decode_global` method into a function~
- Patch ~7~ 6: Pull `insert_pair` method out of `Map` trait into newly create `Insert` trait
Resolves: https://github.com/rust-bitcoin/rust-bitcoin/issues/576
(Title of PR is `Make Map trait private` because that is the API break.)
ACKs for top commit:
dr-orlovsky:
ACK dfd8924398
apoelstra:
ACK dfd8924398
Tree-SHA512: 1a78294bc8a455552d93caf64db697f886345ba979f574abad55820415958fee1c2dd16945f4eafdbe542fa202cb7e08618aa137ec7ee22b3c9dac5df0328157
Tobin Harding
Andrew Poelstra
Andrew Ahlers
sanket1729
Noah Lanson
Dr. Maxim Orlovsky
Riccardo Casatta
Casey Rodarmor
wim-web
Roman Zeyde