Make Map trait private #752

tcharding · 2022-01-04T02:41:52Z

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: #576

(Title of PR is Make Map trait private because that is the API break.)

Kixunil · 2022-01-04T13:39:13Z

Not knowledgeable about the topic, structure of the code looks fine. Tests (as a separate commit) would be really helpful.

apoelstra

ACK 3582f69

concept ACK making this an error. I'm not sure if there are any scenarios where the user might want to remove or replace keys in this map (IIRC the combine logic has some interesting stuff here where if it sees two different paths in one spot, it will always use the longer of the two), but the current "silently replace" behavior is surprising and inconsistent with how we handle e.g. proprietary keys.

I really like the structure of this PR. These cleanups are great. Thanks!

tcharding · 2022-01-04T23:42:40Z

I really like the structure of this PR. These cleanups are great. Thanks!

I tried something new; after your comment @apoelstra over here I've been thinking how to best get docs fixes in in the least invasive manner. I added a couple of patches to the PR that do docs improvements to the code already being touched by the PR.

tcharding · 2022-01-05T02:43:06Z

Tests (as a separate commit) would be really helpful

@Kixunil I can't really work out what needs testing here. The new logic in this PR is trivial, instead of putting foo in one map put it in another one - no point testing that really. Is there something specific you had in mind.

Now whether the logic is correct or not is another matter :)

Kixunil · 2022-01-05T11:48:18Z

My understanding was that it was previously wrong behavior and now it's fixed so write a test that fails for previous code and doesn't for the fixed version. This would make it less likely that future changes break it again.

apoelstra · 2022-01-05T15:39:57Z

Lol! So, there was not actually a behavioural bug at all then. (Indeed, the existing consensus_decode_global does the right thing.) It took me a few reads of the new code trying to figure out "where does the behavior change?" before I finally read your updated PR description.

ACK this PR even though it is now purely a cleanup/refactoring PR.

apoelstra

ACK 4158b23

apoelstra · 2022-01-05T15:48:58Z

Can you change the PR title to reflect its current contents?

dpc · 2022-01-05T17:56:03Z

src/util/psbt/map/global.rs

-
-        Ok(())
+    fn insert_pair(&mut self, _: raw::Pair) -> Result<(), encode::Error> {
+        // This method is only called during deserialization, for PSBT the insertion-like logic is


I don't understand this comment, therefore I suspect it could be made more clear. Please remember that it's more necessary and helpful for people who do not understand what is going on, than to people that already do.

I understand that you figured out that this is never used, but I just don't understand why it is never used. And (often recrusive) "why" is the most important question to answer. Is it some accident, or are there deeper fundamental reasons behind it?

BTW. Implementation of a trait(interface) method that returns a "not implemented/needed" error is often a sign that the interface is incorrect. Possibly trait Map should be split into trait Map and trait MapMut : Map or something (named like DerefMut)

I suspect the reason is, as you say, that this Map trait tries to generalize over the different PSBT components but actually they have different logic.

Thanks for your comments @dpc, I'll have a bit more of a play with this and see what I can come up with.

tcharding · 2022-01-06T03:25:18Z

ACK this PR even though it is now purely a cleanup/refactoring PR

Changing the Map trait to private is an API breaking change I believe.

tcharding · 2022-01-06T03:39:15Z

@dpc is totally correct, returning unreachable! is bad form. Sorry lads, sloppy work by me on this one. I have now separated the insert_pair method out into a separate trait and then removed the code entirely for the original implementation of it on PartiallySignedTransaction.

First 4 patches were untouched, please re-review @apoelstra at your leisure since this is not a bug anymore (label can probably be removed from #576).

dpc · 2022-01-06T06:34:52Z

src/util/psbt/map/mod.rs

+pub use self::output::{Output, TapTree};
+
+/// A trait for inserting into a PSBT key-value map.
+pub(super) trait Insert {


Not strong feelings about it, but having two names so different does not seem idiomatic (in a sense the e.g. standard library doesn't do that, AFAICT).

https://doc.rust-lang.org/std/ops/trait.DerefMut.html is a best example, but not exactly like what is happening here.

At very least it seems MapInsert+ Map seems to tie the two together.

Also, unless there's a good use-case for maps one can insert to but not get, trait MapInsert : Map would help the code using these, as T : MapInsert will imply T: Map which will save some typing when both reading and writing is used.

Thanks for your input @dpc, I ended up going with @apoelstra's suggestion below and making insert_pair into functions.

apoelstra · 2022-01-06T16:05:15Z

I'm alright with 68dc0b1 but I'm not convinced that the Insert trait is worth including, rather than just having one-off methods. I agree with @dpc that its name is counterintuitive given that it's something of an extension of Map.

My feeling is that Map exists purely for code-reuse reasons (so that we can implement consensus_encode in the same way for global/input/output maps). Maybe Carl originally intended that we could do decoding as well through this trait, but that was obviously not to be, because the different maps have different weird decoding rules and different fixed types for known fields. He might've also intended that this trait be an easy way for some kinds of users to interact with PSBT maps generically, but since we decided to make this trait private, that's no longer a goal. (I also think that it wasn't ever going to work so well.)

GIven that consensus_encode, as written, depends only on get_pairs, I think that we should drop both insert_pairs and merge from the trait, and rather than re-instantiating them in a new trait, just let them be standalone methods.

tcharding · 2022-01-07T00:00:33Z

Will work in the comments above and re-spin.

tcharding · 2022-01-10T02:44:14Z

re-based on master

apoelstra

ACK 6041461

Looks like a pretty annoying rebase lol.

sanket1729

This is going to conflict with other massive refactor PRs again like #757 and #762

sanket1729 · 2022-01-11T00:48:33Z

src/util/psbt/map/global.rs

-                                    }
-                                } else {
-                                    return Err(Error::DuplicateKey(pair.key).into())
+pub fn consensus_decode_global<D: io::Read>(d: D) -> Result<PartiallySignedTransaction, encode::Error> {


Why remove the pub(crate) visibilty? This is a now public function inside a private mod.
Would it be better to have a pub(crate) or pub(in Path)? I don't know if pub(in path) is supported in MSRV.

Fair point, I tried with pub(in path). Seems my new shell function to build with 1.29 doesn't work today so I'm relying on CI to see if it works.

The first time in my rust experience I finally see the case for this strange in path visibility. Usually you need a cross-mod visibility and in path is not applicable there...

apoelstra

ACK eccb549

dr-orlovsky

Several questions/issues - pls see comments

dr-orlovsky · 2022-01-13T17:24:38Z

src/util/psbt/map/input.rs

-                btree_map::Entry::Occupied(k) => {
-                    return Err(Error::DuplicateKey(k.key().clone()).into())
-                }
+                btree_map::Entry::Vacant(empty_key) => {empty_key.insert(raw_value);}


nit: very unusual formatting not used elsewhere in this library. I will argue to keep the original one in this variant

Ha! Yes this is indeed strange syntax, it was here before me, I also had to look at it twice. Now it has caught you also I'll change them all. FTR the reason is so that the match arm does not return anything, this cannot be done on a single line without this strange usage of braces.

dr-orlovsky · 2022-01-13T17:27:49Z

src/util/psbt/map/global.rs

-                                    }
-                                } else {
-                                    return Err(Error::DuplicateKey(pair.key).into())
+pub fn consensus_decode_global<D: io::Read>(d: D) -> Result<PartiallySignedTransaction, encode::Error> {


The first time in my rust experience I finally see the case for this strange in path visibility. Usually you need a cross-mod visibility and in path is not applicable there...

dr-orlovsky · 2022-01-13T17:30:16Z

src/util/psbt/map/global.rs

-impl PartiallySignedTransaction {
-    pub(crate) fn consensus_decode_global<D: io::Read>(d: D) -> Result<Self, encode::Error> {


I will argue not to change the function into a global one (even with limited visibility).

The whole concept of associated methods not taking self is about namespacing - and I do not see a reason to introduce a large BLOB of consensus-critical code change just to reformat things and have a namespace as a part of a function name and not struct abstraction.

Fair point, thanks. I've removed the patch that made this change. That also requires removing the in path patch also (FTR its the first time I've ever seen it either, I had to look up how to do it :)

PSBT is not consensus code.

tcharding · 2022-01-13T22:10:17Z

Dropped two patches from this PR and force pushed. I'll have to ask you to re-ack please @apoelstra (gee that's the fifth one for you now, sorry about that).

apoelstra

ACK bf09d75

dr-orlovsky · 2022-01-14T09:16:51Z

Needs rebase again :(

tcharding · 2022-01-14T22:57:48Z

ca22374 Refactor match arms now contains changes requested by @dr-orlovsky (re: strange syntax.

Apart from that just rebased on master. Please re-ack @apoelstra and accept my nomination for PR with the most re-acks :)

These imports are unusually placed, from the code comment it seems the reason is stale. Move imports to top of file as is typical.

Refactor the match arms to make the code around the key used for map look up easier read. Refactor only, no logic changes.

Mildly improve the docs by adding full stops to every rustdoc comment.

Improve the function rustdocs in the `psbt::map` module by: - using third person tense as is idiomatic in the Rust ecosystem - using rustdoc `///` not code comments `//` for methods - Use `# Return` section for documenting return values Done for this module only as part of a PR fixing code within this module.

The `Map` trait has been deemed confusing and not that useful to users of the library, we still use it internally within the `psbt` module though so make it visible only in `psbt` and `psbt::map`.

The method implementation of `insert_pair` is currently not used for `PartiallySignedTransaction`. Having an implementation available is deceiving. Delete the unused `insert_pair` code from `PartiallySignedTransaction` (dead code). Make the `insert_pair` methods from `Input` and `Output` be standalone functions.

tcharding · 2022-01-14T23:06:39Z

Fixed error I introduced while re-basing (PsbtSigHashType import). Soon I'll run contrib/test.sh or all patch sets before pushing :(

dr-orlovsky

ACK dfd8924

dr-orlovsky · 2022-01-15T10:10:48Z

src/util/psbt/map/output.rs

-    /// Returns the inner as Err if it is not a complete tree
+    /// Converts a [`TaprootBuilder`] into a tree if it is complete binary tree.
+    ///
+    /// # Return


nit: AFAIR it is usually named "Returns"

doh, you got me! I'm going to leave it as is for 2 reasons

Constant docs fixes are high on my priority list while I'm finding my feet in this project, so this will get done

This PR has been doing cycling for ages and now has 2 acks, I'll do a follow up

no, for sure do not update it discarding reviews

apoelstra

ACK dfd8924

(6 commits, for future reference ... range-diff is easier to run if you know this value)

Kixunil added the API break This PR requires a version bump for the next release label Jan 4, 2022

apoelstra previously approved these changes Jan 4, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 39614a0 January 4, 2022 23:36

tcharding force-pushed the 576-psbt-duplicate-fields branch from 3582f69 to 39614a0 Compare January 4, 2022 23:36

tcharding force-pushed the 576-psbt-duplicate-fields branch 2 times, most recently from b76e370 to 4158b23 Compare January 5, 2022 03:59

tcharding marked this pull request as ready for review January 5, 2022 04:03

tcharding requested a review from apoelstra January 5, 2022 04:03

apoelstra previously approved these changes Jan 5, 2022

View reviewed changes

dpc reviewed Jan 5, 2022

View reviewed changes

tcharding changed the title ~~Disallow duplicate fields in PSBT Global map~~ Make Map trait private Jan 6, 2022

tcharding dismissed apoelstra’s stale review via 68dc0b1 January 6, 2022 03:35

tcharding force-pushed the 576-psbt-duplicate-fields branch from 4158b23 to 68dc0b1 Compare January 6, 2022 03:35

tcharding requested a review from apoelstra January 6, 2022 03:42

dpc reviewed Jan 6, 2022

View reviewed changes

tcharding force-pushed the 576-psbt-duplicate-fields branch 2 times, most recently from 861f08a to 6041461 Compare January 10, 2022 02:43

apoelstra previously approved these changes Jan 10, 2022

View reviewed changes

sanket1729 reviewed Jan 11, 2022

View reviewed changes

tcharding force-pushed the 576-psbt-duplicate-fields branch from 33f7b30 to eccb549 Compare January 13, 2022 03:31

apoelstra previously approved these changes Jan 13, 2022

View reviewed changes

dr-orlovsky reviewed Jan 13, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via bf09d75 January 13, 2022 22:10

tcharding force-pushed the 576-psbt-duplicate-fields branch from eccb549 to bf09d75 Compare January 13, 2022 22:10

apoelstra previously approved these changes Jan 14, 2022

View reviewed changes

dr-orlovsky added this to In progress in PSBT via automation Jan 14, 2022

tcharding dismissed apoelstra’s stale review via 112169c January 14, 2022 22:56

tcharding force-pushed the 576-psbt-duplicate-fields branch from bf09d75 to 112169c Compare January 14, 2022 22:56

PSBT automation moved this from In progress to Review in progress Jan 14, 2022

tcharding added 6 commits January 15, 2022 10:02

Move imports to top of file

e6af569

These imports are unusually placed, from the code comment it seems the reason is stale. Move imports to top of file as is typical.

Refactor match arms

11c046b

Refactor the match arms to make the code around the key used for map look up easier read. Refactor only, no logic changes.

Add full stops to rustdocs

92059c2

Mildly improve the docs by adding full stops to every rustdoc comment.

Make Map trait private to psbt module

ad75d51

The `Map` trait has been deemed confusing and not that useful to users of the library, we still use it internally within the `psbt` module though so make it visible only in `psbt` and `psbt::map`.

tcharding force-pushed the 576-psbt-duplicate-fields branch from 112169c to dfd8924 Compare January 14, 2022 23:04

dr-orlovsky linked an issue Jan 15, 2022 that may be closed by this pull request

Remove unused last argument in impl_psbt_get_pair macro #754

Closed

dr-orlovsky approved these changes Jan 15, 2022

View reviewed changes

dr-orlovsky added this to the 0.28.0 milestone Jan 15, 2022

dr-orlovsky removed a link to an issue Jan 15, 2022

Remove unused last argument in impl_psbt_get_pair macro #754

Closed

apoelstra approved these changes Jan 15, 2022

View reviewed changes

PSBT automation moved this from Review in progress to Ready for merge Jan 15, 2022

sanket1729 merged commit 093f8b6 into rust-bitcoin:master Jan 16, 2022

PSBT automation moved this from Ready for merge to Done Jan 16, 2022

tcharding deleted the 576-psbt-duplicate-fields branch January 17, 2022 21:23

		impl PartiallySignedTransaction {
		pub(crate) fn consensus_decode_global<D: io::Read>(d: D) -> Result<Self, encode::Error> {

Make Map trait private #752

Make Map trait private #752

Conversation

tcharding commented Jan 4, 2022 • edited

Kixunil commented Jan 4, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jan 4, 2022 • edited

tcharding commented Jan 5, 2022

Kixunil commented Jan 5, 2022

apoelstra commented Jan 5, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 5, 2022

dpc Jan 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jan 6, 2022

tcharding commented Jan 6, 2022

dpc Jan 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 6, 2022

tcharding commented Jan 7, 2022

tcharding commented Jan 10, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a 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

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky Jan 13, 2022 • edited

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

tcharding commented Jan 13, 2022

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 14, 2022

tcharding commented Jan 14, 2022

tcharding commented Jan 14, 2022 • edited

dr-orlovsky left a 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

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jan 4, 2022 •

edited

tcharding commented Jan 4, 2022 •

edited

dpc Jan 5, 2022 •

edited

dpc Jan 6, 2022 •

edited

dr-orlovsky Jan 13, 2022 •

edited

tcharding commented Jan 14, 2022 •

edited