Is there like a BIP or something that can be referenced here?

Nah, this is all super old stuff. Might be somewhere on wiki.bitcoin.it, but mostly just Core's ref impl.

Should contain the explanation that this is because of usize (rightfully) not implementing Into<u64> but we don't support architectures other than 32 and 64 bits.

On it. Yeah it's pretty unfortunate. Also not sure what to do with usize at all, see todo.

I believe we already do have the check but we were discussing elsewhere that these checks should be tied with the functions they provide for clarity.

I added a NB instead, do you think that's enough at this point? Or should I add a unit tests in this module specifically to check that usize doesn't exceed 64 bits?

It should be more along the lines of:

#[cfg(not(any(target_pointer_width = "32", target_pointer_width = "64")))]
compile_error!("Only 32 and 64 bit architectures are supported");

Which I think we have somewhere but it's far away. Ideally we would have a module (or extension trait) with conversion functions that contains that check and then we would call into those everywhere and get rid of all as or try_into().unwrap() conversions in the whole codebase.

But for now NB is fine since the code is probably littered with these things anyway.

FTR this makes the trait no longer object safe. Could be a problem for downstream users, so might be worth adding where Self: Sized.

I'm a bit confused about what impl IntoU64 is supposed to mean. I have never seen this used in an argument to a trait method before and this style isn't used anywhere else in this codebase.

It would be clearer to just use a generic. The result will still not be object-safe. To get object safety you'd need to use a dyn type.

That use of impl is just a shorthand to create a generic, these two signatures are equivalent (except the ability of the caller to explicitly specify the generic type using the turbofish):

    fn emit_varint(&mut self, v: impl IntoU64) -> Result<usize, io::Error>;

    fn emit_varint<T: IntoU64>(&mut self, v: T) -> Result<usize, io::Error>;

I can add Self: Sized, but not sure that is sufficient to make the WriteExt types boxable. Let me try.

Hmm, that's pretty annoying as then we don't allow reading from and writing to byte slices anymore as they are ?Sized. Is the boxability really an issue? I would really prefer to not get a dynamic reference in this method.

Thanks for explaining. And no, I don't think Sized is sufficient to make WriteExt boxable. A generic method is inherently not boxable because the vtable needs to hold pointers to actual functions, and the compiler doesn't know upfront how many different functions the generic will expand into.

I think we should sacrifice object-safety. But if we want to keep it, our options are:

• Return to the old code which has a type rather than methods
• Move the method out of the trait definition into a standalone method
• Move the method into IntoU64 rather than WriteExt (and then IntoU64 will not be object-safe but that seems less bad)

• change the signature to take u64 and convert at call-time

But I agree object-safety here shouldn't be important.

I don't see how it's related to slices, can you explain? This method previously wasn't present and the trait was object safe, now the method was added which makes it no longer object safe which is easily fixable by doing where Self: Sized, so that any existing code using &mut dyn WriteExt will continue to work without us losing anything.

Also I suspect we should impl WriteExt for &mut dyn WriteExt and impl WriteExt for Box<dyn WriteExt> but that's off topic.

easily fixable by doing where Self: Sized,

I don't understand this. The problem with object-safety is that we are adding a generic method to the trait. Sized won't help with this.

It will, the offending method will be unavailable in unsized context but all others will be fine so any code using dyn WriteExt today will continue to work.

the offending method will be unavailable in unsized context

ooo TIL. Ok, sorry for causing confusion.

This is cute but does it really make sense to complicate emit_slice by returning usize just because of this? This also repeats so why not have fn emit_varint_prefixed_slice?

That actually exists as consensus_encode_with_size. I should probably use it there.

pub(crate) fn consensus_encode_with_size<W: io::Write>(
    data: &[u8],
    mut w: W,
) -> Result<usize, io::Error> {
    Ok(w.emit_varint(data.len())? + w.emit_slice(data)?)
}

Changed the two other occurrences to use this method.

I'd be ok with changing that, but our Encodable trait also does it and now emit_varint has to do it, so it's not the only remaining method breaking with the trait. Tbh the int ones can also do it to make all the len += pieces more concise, but I didn't do it because it would probably be a ton of changes. I can do it if you prefer.

On second thought, I wouldn't be opposed to moving this consensus_encode_with_size method into the WriteExt trade as something like emit_slice_with_length like you said.

Yeah, moving it here sounds good. But my point is we shouldn't return usize from emit_slice - the length of the slice is already known. Also I wonder if we really want it rather than just using write_all.

In e9123d1d25bce9be4d1702ae9da6b99bd2e9674e:

These can use .into_u64. (Similar below and in several other places.)

In e9123d1d25bce9be4d1702ae9da6b99bd2e9674e:

If we are going to make this pub it should probably live in the crate root (or maybe even in bitcoin-internals).

As @Kixunil suggests we should tailor our compile-gate by e.g. gating the usize impl specifically on usize fitting into u64. Then only code that tries to use this trait on usize will fail to compile on 128-bit arches.

In e9123d1d25bce9be4d1702ae9da6b99bd2e9674e:

It doesn't really matter but I kinda prefer using .into rather than casting for these cases, since it's possible.

Why? This looks completely redundant since the caller can just call .len() on the argument. You've complained about breaking changes and this is a breaking change...

Why? This looks completely redundant since the caller can just call .len() on the argument.

I think it's worthwhile to have a uniform API. You can see at the call sites that it makes the code easier to read.

You've complained about breaking changes and this is a breaking change...

I'd prefer we take the opposite tack and say "we let you make a breaking change so please don't complain about our breaking changes" ;).

More seriously, while this is a breaking change, it's in an obscure method of a trait that basically nobody ever uses directly. So I think the breakage is worth the API cleanup.

"we let you make a breaking change so please don't complain about our breaking changes"

😆 OK.

I think it'd be better to name this varint_size_from_u64 and not mention const. (I know I've made the same mistake in hashes in the past but I learned since then.) Also maybe just not make it public for now.