The annoying thing is re-writing DecoderError to return an enum instead of the default ArrayDecoder error if the bit is out of range..

cACK ; Yeah, true, this is legit!

@Abeeujah what do you think about going with this one then and closing #6297?

Title needs attention please.

Title needs attention please

Thanks, fixed.

Are you sure your claims about the macro not working with custom error types is correct?

cACK ; Yeah, true, this is legit!

@Abeeujah what do you think about going with this one then and closing #6297?

Yeah, sure, would be closing.

Are you sure your claims about the macro not working with custom error types is correct?

Yes, and the reason is that the macro decoder_newtype appears to be inflexible when using an enum instead of a struct for the error type.

In include/decoder_newtype.rs#L100

(err: <$decoder as encoding::Decoder>::Error) -> $err { $err(err) }

Notice $err is using parens (err), which is for a struct, not an enum.

Have you tried defining a map_push_bytes_err function instead of using the default? See AmountDecoder in units/src/amount/unsigned.rs for an example:

#[cfg(feature = "encoding")]
crate::decoder_newtype! {
    /// The decoder for the [`Amount`] type.
    #[derive(Debug, Clone)]
    pub struct AmountDecoder(encoding::ArrayDecoder<8>);

    /// Constructs a new [`Amount`] decoder.
    pub const fn new() -> Self { Self(encoding::ArrayDecoder::new()) }

    fn map_push_bytes_err(e: encoding::UnexpectedEofError) -> AmountDecoderError {
        AmountDecoderError::eof(e)
    }

    fn end(result: Result<[u8; 8], encoding::UnexpectedEofError>) -> Result<Amount, AmountDecoderError> {
        let value = result.map_err(AmountDecoderError::eof)?;
        let a = u64::from_le_bytes(value);
        Amount::from_sat(a).map_err(AmountDecoderError::out_of_range)
    }
}

Have you tried defining a map_push_bytes_err function instead of using the default?

Yeah, that looks like it works. Thanks for the suggestion.

Need to clean it up to make it less hideous looking. Marking as draft until I have a chance to.

Arbitrary generates actual objects, not "inputs". The objects it generates must uphold the invariants of the type.

Arbitrary generates actual objects, not "inputs". The objects it generates must uphold the invariants of the type.

As best as I can tell, these fuzz tests are not using Arbitrary though, are they? It looks like the fuzz test is just generating random byte arrays "data", then calling:

    let old_result: Result<old_bitcoin::p2p::message_compact_blocks::SendCmpct, _> =                             
        old_bitcoin::consensus::encode::deserialize(&data); 

    let new_result: Result<p2p::message_compact_blocks::SendCmpct, _> =                                          
        decode_from_slice(&data);

Then comparing that if one of them creates a SendCmpt successfully, so should they both, and if one errors, so should they both.

So I don't think there is any change required to any Arbitrary type.

the name "old_bitcoin" is pretty funny though I think.

Ah, right, the issue is that you need to backport this change to 0.32.x at the same time as you do this PR. Or you need to whitelist the change in the fuzztest.

In this case I'd suggest backporting, since the change fixes our code to better match the BIP.

If you just want to get this PR in without further rebase hell, you can patch the fuzztest for now but please file an issue that this needs a backport (and the fuzz patch removed).

Ah, right, the issue is that you need to backport this change to 0.32.x at the same time as you do this PR. Or you need to whitelist the change in the fuzztest.

Right, that's exactly what I said a few weeks ago:

That makes sense though, since anything not 1 or 0 will be accepted by old_bitcoin 0.32.x SendCmpct, and now it returns an error. So I guess that means an exception is needed for this fuzz case or backport the change.

Anyway, I agree that this change should be back-ported to 0.32.x, particularly since it's also being patched in core and btcd. Will circle back on this when I have a chance to.

Ok so I see #6457 in flight. Lets just change it there for 0.32.x? I realize that this might require fiddling with scripts and API text files, and I want to be upfront that I would prefer not to deal with rebasing and conflict that goes with dealing the the API text files. Lately with my projects I add files to the examples directory so that I don't accidentally break the API. I realize a project as large as rust-bitcoin can't possibly have an example to use every API, however, I don't think a txt file would help me much from accidentally changing the API. Generally, I find reading the code to more informative than the giant wall of txt files.. Also I don't like feeling like the bad guy that doesn't want to fiddle with the files, by my time is my own, and my contributions are not under any coercion. I mean, if someone wants to send me some sats to run scripts and fiddle with API text files all day then maybe :P. Just want to avoid the conflict that has arisen in the past when I've put in a PR that requires dealing with these files.

#6457 is merged and does not have this PR's change in it. So we will need a separate backport, sorry. Assuming this gets in.

I appreciate that you don't care to deal with API files, but this PR does not change any API surface.

Right, that's exactly what I said a few weeks ago:

For future reference, when you post correct comments like "this has nothing to do with Arbitrary" immediately followed by irrelevant-but-provocative ones like "Arbitrary should generate invalid objects" (or "I don't understand how a CI job that diffs API files can help me notice changes in the API"), the original comment is likely to go unnoticed. Yes, this is my own fault for not reading, but if your goal is to get your PR merged in a timely fashion, you should just get it into a state where it can pass my local CI and move on with your life.

I appreciate that you don't care to deal with API files, but this PR does not change any API surface.

I understand that. That's one of the reasons I still contribute to this crate. There are other changes I'd like to see get in, for example: #6439, although I try not to pickup PRs that will cause the need for fussing with API txt files. The reason I bring it up here is that creating a backport will need to fuss with txt files I presume.

For future reference, when you post correct comments like "this has nothing to do with Arbitrary" immediately followed by irrelevant-but-provocative ones like "Arbitrary should generate invalid objects" (or "I don't understand how a CI job that diffs API files can help me notice changes in the API"), the original comment is likely to go unnoticed.

If you review the order of the conversation, please notice that I said "correct comment" long before "provocative comment about API files", so I don't think you can blame that on the reason this PR has been ignored. From what i've observed, this PR is ignored because nobody seems to care about maintaining this crate. You didn't bother to take the time to see if your hunch about Arbitrary causing the problem was correct, probably because you're overstretched with other more pressing issues. The other "maintainers" seem to not care either. Neither Nick nor Rob ever seem to comment unless prompted and Tobin doesn't seemed to want to be bothered with this crate either.

So, if rust-bitcoin is interested in maintaining this crate, I'll step up and help be a maintainer. However, I'm going to push back on things that I don't think make sense. And maybe you don't like people having opinions other then yours, however I think good maintainers should push back and not just be "yes men". Lastly, I'll say that this API txt thing has been opposed by other maintainers (Kix) and contributors. I'm pretty sure DPW was fairly critical of some policies this project has around tooling when he said "go ahead and fight whatever unproductive tooling wars you want but I'm not following anymore" (paraphrasing).

your goal is to get your PR merged in a timely fashion, you should just get it into a state where it can pass my local CI and move on with your life.

Given that there seems to be not much interest in maintainers reviewing and helping out, I'm pretty unmotivated to continue to pile time into this. I also don't want to waste your time either if these changes are just not important. So, I'm willing to work on it still if it's useful, although I'm also fine to close this and/or let someone else pick it up.

All fair points. It is true that I feel very little urgency to get anything into p2p, which is why I've repeatedly let other things on my never-ending review backlog get in front of it.

Though I don't think it's fair to say that I don't care about the crate, or about this PR in particular, just because I didn't take the time to fix the CI failure. I offered an idea of what might be causing it (which was wrong, but had it been right it might've been nonobvious) and moved on. But ultimately it's the PR author's responsibility, in almost all cases, to get the code to pass CI.

I also suspect that much of the reason that Rob, and Tobin, and everyone else, has ignored this PR is because it has a long conversation between you and me that has not ended in an ACK.

Anyway FWIW I have put #6525 into my merge queue. Once that passes and merges, and we have a new 0.32.10x release, we can update lockfile here to use that one in the fuzztests, and then this will be good to go.

All good. I would only add that I had all CI tests passing. Your fuzz tests are sort of a hidden obstacle that is non-obvious to anyone submitting a PR.

Your fuzz tests are sort of a hidden obstacle that is non-obvious to anyone submitting a PR.

On the one hand, yes. On the other hand, we run them on Github in a daily cronjob, so the breakage would've been apparent shortly after merge. Not that this helps you any, as a PR author.

FTR I did not read the whole conversation here. Please excuse me.

@yancyribbens are you willing to tack a commit onto here which updates the version of bitcoin in fuzz/Cargo.toml to 0.32.102?

Actually I can just do it.

Ah, no, I would need to rebase on master since #6416 changed fuzz/Cargo.toml out from under us. Adding a commit to your PR feels fine, but rebasing it (even though your commits are not signed) feels like it's too agressive an action.

@yancyribbens are you willing to tack a commit onto here which updates the version of bitcoin in fuzz/Cargo.toml to 0.32.102?

Done. looks like the fuzz tests are passing now 🎉