Bring Elements up to date with Bitcoin Core 22.0

[apoelstra]

WIP while I get CI to pass.

[gwillen]

Reviewed up to, but not including, c8d2647 . That took about half an hour to do about 100 commits using my script, which suggests about another 5 hours at this rate, if nothing unexpectedly complex shows up. That's not counting time to deal with whatever fresh changes at the end of the list are not coming from existing PRs.

[gwillen]

Noting the suggested follow-up from a commit mesage:

commit 183caa4bd24123e9395d2d70d8d3ead4443c4d1e
Merge: bc9605a44a 6f2ca726ce
Author: Andrew Poelstra <apoelstra@wpsoftware.net>
Commit: Andrew Poelstra <apoelstra@wpsoftware.net>

    Merge 6f2ca726ce2 into merged_master (Bitcoin PR #20658)
    
    This deletes .travis.yml. In a follow-up commit (to the whole rebase) we should
    re-instate the Liquid-specific CI tests in Cirrus.

[gwillen]

When bringing in bitcoin/bitcoin#20480 , switching from boost::variant to std::variant, it seems like you'd want to switch from boost::get to the directly-analogous std::get, rather than std::get_if, which is the version that can return nullptr on failure, and as a result has to use pointers instead of references. It looks like you are in fact assuming success, since you immediately dereference the returned pointers anyway (in at least one place.) What's up here?

(This was in your merge commit acf709b .)

EDIT: Oh, slight revision to that: It looks like boost::get is overloaded to provide both versions. So it's possible you want std::get when it's used with references and std::get_if when it's used with pointers. If there's not some other reason you did it the way you did instead, I'm happy to poke more at it and propose an alternative version.

[gwillen]

Reviewed up to, but not including, 81f3883 . Rate continues to be about 3 commits / minute, script reports 859 commits left, a bit under 4h45m of reviewing. (I think I eyeballed it slightly low before.)

[apoelstra]

@gwillen get, by using references rather than pointers, does not have the correct signature to use in these contexts. It would be more invasive to switch everything to references (assuming that I even could), though maybe I should've.

boost::get appears to have overloads for both references and pointers, I guess because this is a creative way to make pointer nullability even worse and the boost authors couldn't resist. So the equivalent std method depends on which overload was used.

[gwillen]

@gwillen get, by using references rather than pointers, does not have the correct signature to use in these contexts. It would be more invasive to switch everything to references (assuming that I even could), though maybe I should've.

boost::get appears to have overloads for both references and pointers, I guess because this is a creative way to make pointer nullability even worse and the boost authors couldn't resist. So the equivalent std method depends on which overload was used.

Right, what I'm saying is that it looked like you went the opposite way and switched everything to pointers. In retrospect I'm only seeing a subset of callsites in my diff, but there were a bunch I saw where it looked like the existing code was using boost::get with references, and you switched to std::get_if with pointers, instead of std::get. Possibly you did a few of these before noticing the difference, and didn't go back and undo them? I can find specific instances if you want, but it looked like there were lots.

[gwillen]

Merge https://github.com/ElementsProject/elements/commit/11cbd4bb54af079674c5fc1f0c695c00c7e1e44a into merged_master (Bitcoin PR #17556)
This PR eliminates "strange regtest=0 behavior" in a test which had forced
us to disable the test for Elements. Can re-enable now :)

I also removed the `chain_in_args` parameter to `TestNode`, which Steven added
in https://github.com/ElementsProject/elements/pull/533 (which itself replaces
unconditionally adding chain={} on the command-line, which was added in https://github.com/ElementsProject/elements/pull/458).
These were added in the 0.17 rebase to deal with the job of starting bitcoind,
which then did not support the `chain=` command-line arg as well as elementsd,
which back then required this command-line arg.

This was causing some issues with the "check -acceptnonstdtxn doesn't work on
mainnet" test because it would add -chain=elementsregtest to the command-line
of a daemon that was supposed to be connecting to mainnet/liquidv1. It is
possible to override this behavior, but since 0.20+ versions of elementsd and
bitcoind have essentially the same support for chain= options, it seemed
cleaner to just eliminate the diff.

Beautiful and glorious. I love the smell of special-case hacks getting deleted from the codebase. 👏

[gwillen]

Nit: in d6c85c5, merging bitcoin PR #20832, in the help text for validateaddress, our change from The bitcoin address validated to The address validated got accidentally reversed in the merge.

[gwillen]

In b8d3a6e, bringing in bitcoin #21042 -- upstream removes self.setup_clean_chain = False in a bunch of places, since it's the default. However we had changed this to True in IncludeConfTest. I have no idea why and it doesn't sound like it would matter, especially assuming tests kept passing after this merge. But noting for the record that this merge deleted the line where we had set it to True, so defaulting it back to False, as far as I can tell.

[gwillen]

In bcd5f22, bringing in Bitcoin #20267, upstream splits some tests into --legacy-wallet and --descriptors invocations. For rpc_rawtransaction.py, it looks like after the merge, we actually invoke it three times, once with each of those arguments, but also once with neither. I don't know what that does, I assume the test suite still passes after this change so maybe it just silently does nothing, but it seems like a typo.

[gwillen]

Noting for the record, from 61a241c:

    TODO: determine how Assumeutxo interacts with the fact that we don't save
    out nonces in our normal UTXO serialization. Probably we will need to remove
    the nonce from the CCoinStats serialization to avoid having inconsistent
    hashes across nodes, since these hashes are now checked in assumeutxo?

[gwillen]

Noting for the record in case I have to investigate anything about the GUI later: there was a merge conflict related to the GUI in 2b9625f, merging bitcoin-core/gui#204

(and again in 41743b6 merging bitcoin-core/gui#205 .)

[apoelstra]

@gwillen get, by using references rather than pointers, does not have the correct signature to use in these contexts. It would be more invasive to switch everything to references (assuming that I even could), though maybe I should've.
boost::get appears to have overloads for both references and pointers, I guess because this is a creative way to make pointer nullability even worse and the boost authors couldn't resist. So the equivalent std method depends on which overload was used.

Right, what I'm saying is that it looked like you went the opposite way and switched everything to pointers. In retrospect I'm only seeing a subset of callsites in my diff, but there were a bunch I saw where it looked like the existing code was using boost::get with references, and you switched to std::get_if with pointers, instead of std::get. Possibly you did a few of these before noticing the difference, and didn't go back and undo them? I can find specific instances if you want, but it looked like there were lots.

Sure, I'd appreciate some examples. This was many months ago and I've forgotten what I did. I wouldn't mind opening an issue to audit all these instances after this PR.

[gwillen]

Nit: Seems like some debugging bits maybe got left in a couple places. In d39d57c, merging upstream #20286, there's a new print(output) in test/functional/feature_pak.py that looks like it might not have been meant to leave in (although I don't really know where that even goes in a test.)

And in c4faf2f, merging upstream #20459, an empty if added to src/rpc/util.cpp, and an addition of #include <iostream> that looks like it might have been for debug output.

[gwillen]

Nit: something slightly annoying happened with formatting in a9ef79a, merging upstream PR #21575. It looks like they did a move commit that also changed some formatting stuff for no particular reason, AND conflicted with some changes of ours, and in merging our changes back in we undid the formatting stuff, creating some unintentional bonus diff. Possibly not worth fixing.

[gwillen]

Noting for the record, from the commit message in d51f425, merging upstream #21169:

    Adds fuzztests for RPC. FIXME there is a crash; I just disabled the
    test for now and we can revisit in a future PR.

[gwillen]

For the record, the commit message from d56bdbd, merging bitcoin/bitcoin#21359:

    I hacked up rpc_fundrawtransaction.py a little bit because our `get_address`
    method here depends on nobody having called `createwallet` on a particular
    node ... the "correct" fix for this would be to redo the Elements changes
    to this functional test for the 0.20+ createwallet changes, but because
    there will be big future changes to this test (in Elements #900 (PSET))
    I did the quick/hacky thing, and will defer cleanups to a separate
    post-rebase PR.

(I'm just commenting whenever I encounter a commit message saying some work was deferred to later, so we remember to track that work.)

[gwillen]

I'm going to be a coward and SKIP reviewing 8b34c97 for now -- I'm going to go right past it and not even read it, making a note here to come back to it later. It's definitely the largest diff of any that I have encountered during this entire review, maybe by an order of magnitude, and I'm not going to be able to do it justice at 11PM. I'll come back for it at the end when I'm a little more awake.

[gwillen]

More future-work notes from commit messages, 3ffb9aa:

    Merge c7dd9ff71b9 into merged_master (Bitcoin PR bitcoin/bitcoin#22051)
    
    Does the bare minimum to introduce Taproot wallet support with CT; just
    adds a CPubKey blinding_pubkey to the taproot destination variant and
    updates some visitors.
    
    In future when we define blech32 we will need to make sure we are using
    that encoding and using the pubkey.

Also, there's some added code for CT-Taproot in this commit, which looks reasonable to my eyeball, but I'm not familiar enough with Taproot to really have an opinion about the approach.

[gwillen]

More future-work notes, from e4746d6:

    This PR adds a fuzz test ensuring that all the different destination types
    round-trip to strings (an important test given that Satoshi managed to mess
    this up, having two different address types encode the same way..)
    
    Anyway we fail this test, since both CNoDestination and NullData encode as
    empty strings, and then this "decodes" as CNoDestination (and returns an
    error, but the fuzztest doesn't check that). To make the fuzzer pass, I
    changed NullData to encode and decode as "null".
    
    I think this actually adds some functionality, letting you use "null" as
    an "address" for sendtoaddress and createrawtransaction, thus burning the
    coins and fixing #1011...but this was not my intent and we probably want
    to think a bit more carefully before deliberately supporting this.

[gwillen]

Note: ae15e5d (bitcoin/bitcoin#22405) is where the --enable-glibc-back-compat gets removed from the Guix build, which IIRC impacts us negatively.

[gwillen]

Future-work from 36e5e2a:

    TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
    in documentation and help text even though it was deleted. Need to fix this
    in 0.21 in a separate PR.

[gwillen]

36e5e2a, merging elements#900, is complex enough that it might be nice to get @achow101 to review the merge given that the merged PR is his work. The only thing I definitely noticed was a nit: a stray mention of boost::variant left in a comment in src/util/settings.h.

Also, from the commit message:

    TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
    in documentation and help text even though it was deleted. Need to fix this
    in 0.21 in a separate PR.

@achow101 if you do want to look at this (reviewing the merge forward-porting #900 to 0.22) please let me know and I'm happy to provide any assistance. (I have a script to help me review merges, but I can't find any diff view that doesn't make a hash of this one unfortunately.)

[gwillen]

In 7d1c77f, merging #1002 (which looks fine) I'm curious about something strategy-wise, in terms of how we maintain our diff vs upstream -- why rename the various Taproot constants to _ELEMENTS? I know ideally one would be clear about that, but it adds quite a lot of extra diff that it would be nice to avoid.

@sanket1729 I guess you were the implementer here -- any thoughts on this?

(In general, going through this process makes me wish we had less diff against upstream in some places even though the diff is for a good reason; e.g. all the places we renamed nValue to mapValue or something like that, I kind of would like to undo the rename to reduce the diff a lot, and just leave a comment at the declaration instead.)

[gwillen]

From ba3d786: "In future let's really try to stay up to date with Core." Yeah, I think having backports come in pieces from Core as Elements MRs and then separately as core PRs makes for weird and messy merges and diffs. I'm not sure what our strategy ought to be, but we ought to think and talk about our strategy for this kind of thing if there will be more in the future. (If just doing Elements releases fairly quickly after Core releases would be sufficient that we don't need to backport stuff, I'm all for making that our aim.)

[gwillen]

Future work from 446f764 merging elements#1033:

    This fixes a bug that was eliminated by #22008 -- although a conceptually
    similar one was reintroduced (basically, we do a "test blinding" for fee
    estimation, then potentially delete a change output, then we actually blind
    the tranasction .... but if removing the change output pushes us into an
    edge-case scenario for blinding, Bad Things happen).
    
    Patched in a simple hack. We should fix this properly in a post-22 PR.

[sanket1729]

I know ideally one would be clear about that, but it adds quite a lot of extra diff that it would be nice to avoid.

The values of the constants were changed from bitcoin, which is why I renamed them. I have no opposition to renaming the constants if it makes maintaining elements easier.

[gwillen]

In e4c9cc0, which is back quite a ways, there's an issue I did not catch when initially looking at it -- the &s on some reference parameters got dropped. This caused them to become apparently-unused, which acab524 was attempting to fix. However, they were not unused, and this breaks the original change (and this may be related to the CI issues.)

I will have to figure out why I didn't catch this the first time through, and also what I should do about it -- trying to go back and fix the original merge is probably not worth it.