Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring Elements up to date with Bitcoin Core 0.21 #935

Merged
merged 9,112 commits into from
Apr 6, 2021

Conversation

apoelstra
Copy link
Member

@apoelstra apoelstra commented Dec 3, 2020

Requires a bunch of followup work which I will open other issues for. See also #743 which was our checklist for 0.19.

Then in followup PRs:

Then a couple large followups. Tracking issues:

This caused a segfault because we have no checkpoints. Added a NULL check
which jnewbery suggested on the original PR, but for some reason didn't
get in.
This commit adds signet support, which is a little bit silly/redundant for us :)
Was a surprisingly easy merge to handle, and hopefully in future Core is more
mindful of signed blocks when they are changing code architecture.

I had to change a couple lines of src/signet.cpp to add blank assets to the
CTxOuts and to find transaction input scriptWitnesses. No need to add any
other tx witness data (and the CAsset()s that I did add to make things compile
won't be used..) because signet will always have g_con_elementsmode off.
More RPC cleanups. This one discovered that the `createpsbt` and
`rawblindrawtransaction` RPCs had incorrect argument lists :)
We have a couple RPC hacks in src/wallet/rpcwallet.cpp that were a bit annoying
to adapt, but nothing too bad.

Found a misspelling of `ignoreblindfail` in the `sendtoaddress` args, and a
discrepancy in the `createrawpegin`/`createpegin` args
…r test

Everyone is _not_ connected in this test, so on slow machines
where this check triggers (e.g. the CI boxes) the test incorrectly
fails. This was also a source of (very infrequent) spurious failures
during the rebase.
This was causing build failures on win64, and since we (currently) do
not expose any dynafed or PAK stuff in libelementsconsensus, there is
no reason to be trying to link these.
Should revisit; I am not sure whether we have a long-term stable
URL for prebuilt past Elements releases, but we should be able
to just point test/get-previous-releases.py at that, and change
the set of versions (since we do not have 0.15, 0.19, etc), and
we're good to go.
Mostly harmless signed/unsigned conversions but also an actual
memory leak related to `BlindingData`.
@apoelstra
Copy link
Member Author

Rebased. Removed the bitcoin PRs that were from past the 0.21 fork point, and added the most recent 6 PRs from Elements. Also removed the extraneous xx thing from the fixups.

Everything from 22cf380 (Elements #960 is new).

@gwillen
Copy link
Contributor

gwillen commented Mar 30, 2021

It looks like the last commit that merges a PR into the new branch is 0699c4d, and the fixups are after that. I'm going to create a merged-master branch on this repo, pointing there. (I was using the one on the apoelstra repo before, but it looks like you didn't update it when you did more merging, and anyway we should have it in the main repo.)

@gwillen
Copy link
Contributor

gwillen commented Mar 31, 2021

Looking at the newly-merged commits: copy-paste error in d891120:

             if (!proposed.m_fedpeg_program.IsWitnessProgram(fedpeg_version, fedpeg_program)) {
-                return state.Invalid(false, REJECT_INVALID, "invalid-dyna-fed", "proposed fedpegs program must be native segwit scriptPubkey");
+                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "invalid-dyna-fed", "proposed signblockscript must be native segwit scriptPubkey");
             }

(New error message refers to signblockscript instead of fedpeg script. (I assume "fedpegs program" was a typo.))

I think it's your call whether you want to make this a fixup commit, or try to fix it in-place, given that it's fairly shallow at this point. (This would require rebasing the fixups, obviously, but the rebase should be trivial.)

I think it ought to be possible to fix this in-place with an interactive rebase, without having to redo any of the subsequent merges, as long as the fix doesn't overlap with any of them. This might be worth trying as an experiment, just to figure out whether we can, but I'm also fine with leaving that for future work. (I have never successfully rebased a merge before.)

@gwillen
Copy link
Contributor

gwillen commented Mar 31, 2021

Reviewed the new Elements MR merges. Everything looks good except for the comment above.

@apoelstra
Copy link
Member Author

@gwillen sounds good, re merged-master

As for the copy/paste error, oops. For what it's worth, this wasn't a traditional "copied a bunch of code then failed to edit it" error, this was me copying one of a dozen error lines and picking the wrong one :P. In any case I think we should fix it in a followup PR because this one is a giant pain in the ass to work with, run CI on, etc.

@gwillen
Copy link
Contributor

gwillen commented Apr 1, 2021

For the benefit of the peanut gallery: andrew is seeing a test error that doesn't reproduce locally, so there will be some noise trying to pin it down in CI.

@stevenroose
Copy link
Member

I looked at all the recent changes up until 291da8b and can utACK that. Good to see CI entirely working!

It's still all a bit magical, I'll go over the list of my "just check on that to make sure nothing is accidentally broken" items after this is merged.

If you can ack, I can merge it, @gwillen .

@gwillen gwillen changed the title WIP: Bring Elements up to date with Bitcoin Core 0.21 Bring Elements up to date with Bitcoin Core 0.21 Apr 6, 2021
@gwillen
Copy link
Contributor

gwillen commented Apr 6, 2021

utACK from me!

With the understanding that there's going to be a bunch of followup work, of course.

@stevenroose stevenroose merged commit a49c016 into ElementsProject:master Apr 6, 2021
@apoelstra apoelstra deleted the 2020-12--rebase-0.21 branch April 6, 2021 16:43
stevenroose added a commit that referenced this pull request May 6, 2021
f2a0050 primitives: do not de/serialize asset issuance in bitcoin mode (Andrew Poelstra)
9c1de4d ci: give more memory to a couple Cirrus jobs (Andrew Poelstra)
40c09f6 rpc: fix error message accidentally changed in 5e62edc (Andrew Poelstra)
adbd9f1 qt: fix PSBT/PSET change from 4839db8 (Andrew Poelstra)
057237b qt: fix double-scoping of enum introduced in c3b6bbb (Andrew Poelstra)
5819f25 bitcoin-tx: add ASSET to the help text of some option (Andrew Poelstra)
1f3cfb4 chainparamsbase: fix bool-vs-enum mistake in 9c3480f (Andrew Poelstra)
2573747 fix test code duplication from 3a0de44 (Andrew Poelstra)
0045caf rpc/mining: restore code erroneously dropped in 0e2c963 (Andrew Poelstra)
39112e1 correct copy/paste error in error message (Andrew Poelstra)

Pull request description:

ACKs for top commit:
  stevenroose:
    utACK f2a0050

Tree-SHA512: 5ce831ef24e51e76b93cd4d8e345a382dbd716e38c99565a7c2f25b5eeb8bac1e36c623491c5763ceb7b47444024b5821a878942df150bb01b33eeae33f292b1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants