Disable Serde's default-features

[ass3rt]

With this patch, existing users of the use-serde feature will no longer be
compiling with serde/std enabled, but this allows dependent projects
to import serde and enable serde/alloc as required by some no-std targets.

[ass3rt]

Seems I have some test failures to look into. I'll try to get these worked out tonight.

[tcharding]

Changing features makes this a breaking change, right? We are in the process of a big breaking release anyways but thought I'd flag it.

[ass3rt]

Changing features makes this a breaking change, right?

That is my understanding 👍

[apoelstra]

ACK f18c42c

Yes, this is a breaking change, but it's fine to bring into the RC.

[dr-orlovsky]

NACK: build is broken due to test dependency issues + possible undesirable side-effect of always having non-std serde even for std builds

[ass3rt]

Sorry for the repeated test failures. I promise I am running ./contrib/test.sh before submitting patches, but the tests are passing on my system. Perhaps I'm missing a step. If anyone has a recommended test workflow before submitting PRs, I will happily accept any advice to improve my testing :)

[apoelstra]

There are probably some environment variables you need to set to get test.sh to do anything. In particular DO_FEATURE_MATRIX=true.

[Kixunil]

The test failures are actually caused by the lack of serde-std feature. We really have to use it since I don't believe anyone here wants to bump MSRV to 1.60.

[ass3rt]

I've reworked this PR to address comments, fix tests, and rebase onto master. Sorry for the delay (its been a busy week on other projects).

Specifically, I added the serde-std feature for those that need it, and enabled it for tests.

[Kixunil]

This doesn't look right, the feature is not used anywhere. Did you forget to commit some files?

[ass3rt]

This doesn't look right, the feature is not used anywhere. Did you forget to commit some files?

The only place I am aware of that requires serde-std is in tests, which I added. Are you aware of anywhere else that requires it?

Keep in mind, any feature which enables serde-std will break no-std builds (obviously), so IMO it should not be a requirement for any code which we expect to be no-std compatible

[Kixunil]

Sorry, it looks like I was confused and you don't need serde-std, just call tests with use-serde,std or use-serde,no-std (which is broken because of #947).

[ass3rt]

Ok, I'll back out serde-std. It was a useful thought experiment, if nothing else :)

[Kixunil]

Also to address @dr-orlovsky concern: users needing serde/std can and should enable it explicitly when depending on serde.

[ass3rt]

Oops I forgot to run the dep_test. I did have environment variables set to enable that test, but I must've cleared them before my last push 🤦.

I forgot to explicitly add serde with default features as a dependency for the dep_test. Fixed and force pushed. Tests are passing again. Sorry for all the churn!

[Kixunil]

Oh, shit. I got confused. For some reason the compatibility issue can't be triggered from normal cargo test, only from a dependency and I also forgot to set the env var. 🤦‍♂️

To clean up my shit, I looked at it properly and found a proper solution. Wanted to make a PR against your repo but was unable to do so. Here are my changes: Kixunil@28bb060

Explanation:

I started re-adding serde-std but then I realized the affected methods actually re-implement the default behavior (the existing ones already forward!) so I could just delete them. It looked like that was the only change required but then I noticed in case of script we can get perf benefit from overriding. I then realized we don't need serde-std but serde-alloc and soon after I realized we can depend on it unconditionally because the whole library already depends on alloc and I don't think it will stop depending on it any time soon. (Maybe bitcoin-units will one day.) Thus I just added alloc to the serde features and kept the other changes (even though they are off-topic).

If you want to be super-pedantic and make the history nicer you could split it up into three commits (changing serde features, removing default impls, adding override) but I personally wouldn't block the PR if you don't want to.

Suggested commit descriptions:

0. same as now
1. Removed reimplementations of default methods. (next paragraph) The default methods do the exact same thing thus our overrides are useless, potentially even problematic.
2. Override default visit_byte_buf on Script. (next paragraph) This override may avoid allocation and thus make the deserialization faster.

[ass3rt]

@Kixunil good finds! I like that approach. I am afk for several hours today, but I found the github setting to let others push commits to my PR. You should be able to push now. If not, I'll pull your changes in when I get a chance this evening.

[Kixunil]

Cool, pushed! Do you want to do the squashing?

[Kixunil]

Oh, shit, MSRV issues. Suggested solution: remove the optimization and alloc dependency and reintroduce them after MSRV bump.

Now I finally understand why serde didn't do std = ["alloc"] but conditions everything using any(feature = "std", feature = "alloc").

[Kixunil]

Let's just wait for #964

[apoelstra]

We have #964 now -- can you rebase?

[ass3rt]

We have #964 now -- can you rebase?

Awesome. Done ✔️

[Kixunil]

Could you clean up the git history please? Ideally make it three commits: 1. the topic of this PR, 2. removing unneeded method impls, 2 adding the optimization. But I can accept squashing to a single commit.

[ass3rt]

@Kixunil good call. Done. Thanks for all your assistance!

[Kixunil]

Perfect, thanks!

ACK 76fcf81

[tcharding]

ACK 76fcf81

[apoelstra]

ACK 76fcf81