Pull Request Test Coverage Report for Build 13927232201

Details

• 69 of 162 (42.59%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 82.816%

Totals
Change from base Build 13923187550:	-0.3%
Covered Lines:	21827
Relevant Lines:	26356

💛 - Coveralls

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

I have a few questions/nits but they can be addressed as follow ups if needed.

• buf module may be better named owned to mirror the script modules
• Same as script I would have expected all the AsRef etc to be in mod.rs
• There are a few differences in the owned vs borrow API that I don't see the reason for eg iter(), serialize()
• So that it is easier to read side by side buffers and see the difference/similarities I rekon it would be preferable if the two modules layed out the functions in the same order.

• buf module may be better named owned to mirror the script modules

The reason I went with buf is because I intend to add another owned type backed by ArrayVec. I just didn't want to make the PR too big. For a while I also wondered whether ToOwned::Owned should be the current buf type or the new one until I realized how it interacts with Cow.

• Same as script I would have expected all the AsRef etc to be in mod.rs

I didn't want to move too much code around at first, to not make the review too boring. We can do it later.

• There are a few differences in the owned vs borrow API that I don't see the reason for eg iter(), serialize()

Frankly, I mostly focused on getting some reasonable things working. I'm pretty sure the new type can have richer API. But I also want to make some changes after #4200 merges (IIRC these PRs shouldn't conflict). However, I can already answer iter - the method was available on the Buf type via Deref coercion already, so the APIs are in fact more similar than you think (also funny thing is I had to add the method for the commit changing Target to be non-disruptive).

• So that it is easier to read side by side buffers and see the difference/similarities I rekon it would be preferable if the two modules layed out the functions in the same order.

Because of the above, I think it's more pragmatic to compare docs, not the code.

the method was available on the Buf type via Deref coercion already, so the APIs are in fact more similar than you think

Oh interesting, I had a play and this means that there is no reason to have any of the methods on TaprootMerkleBranchBuf that exist on TaprootMerkleBranch that take &self. For example, there is no need for len (I removed it and ran this test

#[cfg(test)]
mod tests {
    use super::*;
    use crate::hashes::sha256;

    #[test]
    fn deref_coercion() {
        let h = sha256::hash(b"some arbitrary bytes");
        let t = vec![TapNodeHash::from_byte_array(h.to_byte_array())];

        let b = TaprootMerkleBranchBuf::from_collection(t).unwrap();

        println!("len: {}", b.len())
    }
} 

The exact same argument applies to ScriptBuf and Script and it looks like we did indeed do this over there (both is bitcoin and primitives).

@apoelstra you can make the call whether on not to have the privacy_boundry module. Its a trivial matter, not a hill worth dying on. My ACK stands either way you call it.

Regarding methods, for ScriptBuf IIRC we did len because Vec also has it despite it being on the slice. I don't know why but since consistency here has trivial cost I just added it and didn't try to figure it out.

Other than that, I think we should remove the other excess methods. I will do that in a followup PR.

Your question about privacy_boundary module suggests you're still unconvinced by my argument. Is there anything particular you have in mind? It's trivial matter but I am interested in knowing if there is a good counterargument or what I messed up in my explanation. :)

Oh I see the misunderstanding. The module comment makes no sense to me and I spent the whole time fixating on it.

/// Makes sure only the allowed conversions are accessible to external code.

Perhaps just use the same name we used in units::amount::unsigned - encapsulate. And if you really want a comment you say 'Ensures no other code can access the private inner slice'. Or 'encapsulates the inner slice'.

Or if you really like privacy_boundary then change the one in unsigned but I rekon encapsulate well describes what the module does.

I couldn't remember what name we were using and forgot to look it up later. :D

Do you want this in the same PR? (I'll need to leave in a minute for hopefully less than an hour if it helps with deciding.)

Follow up PR is fine bro - thanks.

Done reviewing ef8c9fb.

Needs rebase because I merged the weekly format PR before reviewing this one (we skipped last weeks' format PR and at some point we just gotta do it).

I agree with Tobin's comments that this should be more uniform with script. I'm fine doing that later. At some point either script or this module will be pushed into 1.0, and then we should audit the other one to make sure it looks the same as the stable one, and we can rearrange code then.

As for the privacy_boundary, I think it's fine. I agree that in this case it provides zero value because we have the _unchecked accessor, but it does work as a lint against using .0 which is one of our code guidelines; it lets us try to remove accessors and immediately see what breaks; and it lets us add unsafe markers and have the compiler highlight what parts of the code need to be carefully checked.

And even if none of those things apply here, in general I'd like to have privacy boundaries like this and I'd prefer we be uniform about it.

but it does work as a lint against using .0 which is one of our code guidelines

I think that already has non-zero value. :) My main reason to have this guideline is to allow changing the fields (adding PhantomData etc) without changing ton of lines and as a result we get fewer merge conflicts.

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

I was thinking of moving the trait impls in another commit given that I had to rebase anyway but I'll definitely want to clean up the macro for multiple sizes which isn't as trivial and I have more urgent things to do today.

Looks good -- will run local CI on 9ea2e92.

I'd like to have privacy boundaries like this and I'd prefer we be uniform about it.

If you need to rebase again though can you rename the module though to be called encapsulate to match what's done in amount/signed.rs and amount/unsigned.rs.

IIRC we have such modules elsewhere and they are called differently. I'll open an issue about it to decide on the module name.