Scurbber crate

[arminsabouri]

Migrated from payjoin/concurrent-psbt#32
Implementation of payjoin/multiparty-protocol-docs#9.

Key type bytes overlap across PSBT map types. For example, 0x06 is TX_MODIFIABLE in
global but BIP32_DERIVATION in inputs. So filtering requires map context.

Buffers the global map to detect version and counts (INPUT_COUNT/
OUTPUT_COUNT for v2, UNSIGNED_TX parsed via Transaction::consensus_decode
for v0), then streams remaining maps with per-map allowlists for global,
input, and output.

Pair::decode is pub(crate) in psbt-v2 0.3.0; worked around with a
PairDecode extension trait replicating the upstream logic.

===

Follow up todo that I totally forgot about is to upstream visibility changes to decode upsteam to rust-psbt

[nothingmuch]

i edited the PR text to link to the correct original PR

[nothingmuch]

just nits really, the code makes sense, the scrubbing logic is clear and easy to follow

[nothingmuch]

decode takes a io::Read but this takes a &mut vec, which seems inconsistent...

since these are private you could make the argument that the abstract trait is superfluous, but i think it actually helps clarity, but if you'd rather keep this infallible then please make decode consistent with it (i.e. only using buffers)

IMO either is justified, since (valid) PSBTs are size constrained, and the high level API assumes it is already buffered then all of the streaming logic is not really required, but otoh it's not like it adds much complexity and decode cannot be infallible as the buffer may be contain truncated data

[arminsabouri]

I would rather just keep these consistent

[nothingmuch]

yeah consistency is my only request here, i'd be happy with both consistently &[u8] / &mut [u8] or &mut Vec<u8> (i prefer the former FWIW) or consistently io::{Read,Write}

[arminsabouri]

Ended up going with io::{Read,Write} and kept the infallible expects()

[nothingmuch]

that feels weird, io::{Read,Write} will in general not be infalliable, so if someone would use the API to write to a socket directly, which the type would allow, that may result in the process crashing.

I don't feel strongly either way, seems simpler to avoid the Result type since PSBT data will always be bounded (and scrubbed data will always fit in a buffer smaller than the input buffer), so i think it's perfectly fine to go with an infallible API, but in that case it should probably use slice refs, not the trait.

But if we want this to be more tolerant of broader range of readers/writers, unfortunately io::Read,Write hard code std::io::Error so that can't be statically asserted to be uninhabitable (like std::convert::Infallible), which would make this API inherently panicky unless it returns a Result instead of expecting.

[arminsabouri]

that feels weird, io::{Read,Write} will in general not be infalliable,

True, IIRC thats why I orignally had &mut vec.

so if someone would use the API to write to a socket directly,

i am not sure this is possible. decode/encode are both private methods. Perhaps if we allowed the callers to pass in readers and writers to scrub

but in that case it should probably use slice refs, not the trait.

Sure, makes sense. I think we can get away with this for now. I will update both encode and decode

[arminsabouri]

&mut [u8] for decode needs a write cursor (boilerplate that feels superfluous) or it can return the bytes for scurb() to extend (impl asymetry between encode and decode).

Eitherway a &mut Vec still seems like a natural fit to me for decode. But it does introduce the asymetry back. We could make encode take &mut encode, but thats unnececarry allocs.

Pushed the version where caller keeps track of position in the buffer

[nothingmuch]

since scrubbing by design guarantees that the output buffer will be shorter or equal to the input buffer, it's possible to preallocate a Vec and give a slice to it as a buffer. PSBTs are self terminating so the trailing 0 bytes are immaterial even if the total number of bytes written is not given to the caller, so a Vec provides an advantage there.

&mut [u8] for decode needs a write cursor (boilerplate that feels superfluous)

it is superfluous:

use std::io::Write;

fn main() {
    let mut buffer = [0u8; 12];
    let mut cursor = &mut buffer[..];
    
    println!("remaining: {}", cursor.len());
    write!(cursor, "hello ").unwrap();
    println!("remaining: {}", cursor.len());
    write!(cursor, "world").unwrap();
    println!("remaining: {}", cursor.len());
    
    println!("{:?}", &buffer[..])
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=3cc9c1ab2f257d94ab2a074bdca0ffab

Eitherway a &mut Vec still seems like a natural fit to me for decode. But it does introduce the asymetry back. We could make encode take &mut encode, but thats unnececarry allocs.

i don't see that as an asymmetry. &[u8] is a region of memory to read from, &mut [u8] is a region of memory and a position to write to the way io::Write is implemented on it, and &mut Vec<u8> is a (growable, relocatable, preallocatable, not that we really care) region of memory, a position to write to and an implied length that is stored in the data structure itself.

[nothingmuch]

derp, an explicit length. &mut [u8] is the one with an implied length (but per the example that is readily calculated)

also note that what i said about trailing 0 bytes is not correct, if the last output map's ends in 0x00 bytes (e.g. the value of the last key value pair does), stripping trailing ones may truncate the PSBT, and finding the actual terminating byte would require parsing

[nothingmuch]

this has the same syntactic overhead as a match statement but doesn't let the compiler exploit the fact that it's just a hard coded list

i don't think this will ever make a difference for performance, linear scan of such a small array will be very fast but it feels a bit odd to have an enum but then treat the enum as just a namespace for constants (basically equivalent to a mod { pub const FOO = ... })

[arminsabouri]

Yeah this hybrid const + enum is wierd. I think we can get away with just using matches! . and keep the contains() internal fn call

[nothingmuch]

wouldn't a match block make contains and by extension the vec itself redundant? the enum provides the same values statically at compile time

[arminsabouri]

Yeah. Vec removed

[bc1cindy]

worth asserting the drop direction too?

(e.g. an input 0x06 (BIP32_DERIVATION) is scrubbed even though global 0x06 (TX_MODIFIABLE) is kept)

[arminsabouri]

The scurbber is completly blind to modifiable flags but I think still worth adding. Especially if we end up using the upstream methods in the future (e.g decode)

[nothingmuch]

scrubbing those fields is correct in that setting, in fact that is the whole purpose of this spec existing - the narrow goal to be able to share essential information about the transaction structure without leaking the fields that should stay local/private (but of course scrubbing like this has more uses in other settings)

[nothingmuch]

(to be clear, 'that setting' = during the modifiable phase of a psbt's lifetime)

[arminsabouri]

Test added. Thanks

[bc1cindy]

utACK

[nothingmuch]

urgh damnit, i again thought i had a typoed branch name and pushed it accidentally, but it turns out when i went to "fix" it i deleted your branch, sorry @arminsabouri

[nothingmuch]

what in the fuck, the previous one i deliberately pushed, but this last push was again the result of jj git push --all or something, sigh.

if it happens again you can revoke my privileges without explanation¹ ;-)

Footnotes

1. because i will have already given one myself ↩

[nothingmuch]

Only nits.

Please consider splitting for clarity, i suggest:

• scrub.rs - pub fn scrub at the top, supporting code below
• fields.rs - the various enums
• decode.rs - keep the borrowed code separate

[nothingmuch]

just observing that in principle this could be impl TryFrom<u8> for GlobalInsensitive and then try_from(v).is_ok()

right now this type is private and there isn't much use for it, but i think for the gui stuff it would be useful to be able to statically query information about which fields are considered insensitive, so it may be helpful to make this pub. the main advantage of a try_from over a contains is that this gives the values static names that can be Displayed, but it's not clear to me that that is really necessary as compared to just contains(u8) -> bool.

[arminsabouri]

Hmm this is a really good point. Mind as well bite the bullet and just do it from the get go

[nothingmuch]

would help readability if these constants were named

[nothingmuch]

bikeshedding: move to the top. this function is the main event as far as implementation and the pub entry point to the api but it's relegated below a bunch of private stuff used in its implementation

[nothingmuch]

wouldn't

Suggested change

	write_byte(&mut w, 0x00);
	write!(w, 0x00);

dtrt?

[nothingmuch]

since the backing store is a vec already, not an arbitrary slice, i think using that internally as the io.Write impl is still cleaner?

Suggested change

	let mut out = vec![0u8; psbt.len()];
	let mut out = Vec<u8>::with_capacity(psbt.len());

then just use out instead of w throughout, and no need to calculate the length or truncate at the end. still just one alloc.

&mut [u8] is useful when a buffer is already given by the caller, i.e. pub fn scrub(psbt: &[u8], out: &mut u8) but i think your approach of returning Result<Vec<u8>, Error> is cleaner than my previous suggestion

[nothingmuch]

note that if we ever support a no_alloc version this may become useful again

[arminsabouri]

Ack agreed. Then we can also use out.push(0x00); instead of write_byte(&mut w, 0x00);.

[arminsabouri]

but i think your approach of returning Result<Vec, Error> is cleaner than my previous suggestion

specifically in encode_pair() ?

[nothingmuch]

no only talking about scrub, whether out: &mut [u8] arg vs. -> Result<Vec<u8>> (internal doesn't really matter except for readability)

[arminsabouri]

The job was not started because recent account payments have failed or your spending limit needs to be increased. Please check the 'Billing & plans' section in your settings

:(

[nothingmuch]

The job was not started because recent account payments have failed or your spending limit needs to be increased. Please check the 'Billing & plans' section in your settings

:(

yet another reason to make the repos public, i guess it's time to tear the bandaid off

[bc1cindy]

utACK 0810344

[nothingmuch]

UnexpectedEof typically implies truncation, but this is the opposite, I would add another variant UnexpectedTrailingBytes as that feels clearer but if there is a technical argument for just a single variant for both conditions i don't feel too strongly about this

[arminsabouri]

Thats fair. Fixed

[nothingmuch]

ordering is not currently covered by the BIP but i think it ought to be, see fungi-protocol/docs#12 (review)

it could be implemented with relatively minimal changes by just buffering every map into a btreemap<(type, keydata),value>

this would also allow erroring on buggy serializers that may emit duplicate key value pairs, which a streaming approach does not detect

[arminsabouri]

scrub should preserve the original ordering. Pairs are decoded and encoded in the order they are currently in.
is the main issue duplicate key value pairs?

[nothingmuch]

i posted a rationale here: fungi-protocol/docs#12 (comment)

tl;dr if library a serializes in one order, and library b does in another, for example psbt_v2 will do that predictably because of static field ordering in its structs, then the resulting PSBT will reveal the wallet software being used

if the goal of scrubbing is to make the information revealed in multiparty setting canonical and therefore unidentifying, ordering factors into that