RFC: Deprecate libconsensus

theuni commented at 4:51 pm on January 5, 2024: member

This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway.

See for example the discussions: https://github.com/hebasto/bitcoin/pull/41 #29123

And here: #29180 Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations.

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.

If there are any users currently using libbitcoinconsensus, please chime in with your use-case!

Edit: Corrected final release to be v27.

DrahtBot commented at 4:51 pm on January 5, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, fanquake
Concept NACK	luke-jr
Concept ACK	stickies-v, jamesob, sanket1729, ajtowns, BrandonOdiwuor, jaonoctus, vasild, brunoerg
Stale ACK	jonatack, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

TheCharlatan commented at 5:00 pm on January 5, 2024: contributor

Concept ACK

theuni force-pushed on Jan 5, 2024

hebasto commented at 5:06 pm on January 5, 2024: member

Concept ACK.

dongcarl commented at 5:17 pm on January 5, 2024: contributor

Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus

Ping @apoelstra @tcharding

1thales commented at 5:30 pm on January 5, 2024: none

Ping @Davidson-Souza

stickies-v commented at 5:31 pm on January 5, 2024: contributor

Concept ACK

Davidson-Souza commented at 6:59 pm on January 5, 2024: none

Thank you, @1thales, for the ping.

I’m building a lightweight Bitcoin full node called Floresta, and I’m using libbitcoinconsensus through rust-bitcoinconsensus. I would rather not reimplement the entire script interpreter logic, given the complexity of keeping this code consensus-compatible with core. I did reimplement some consensus logic, but tx validation and script interpreter are the hardest ones to. libbitcoinconsensus has some issues, for example libbitcoinconsensus doesn’t validate relative locktimes like CSV. I would prefer validating everything, including those.

Furthermore, I’ve exchanged some emails with @TheCharlatan about libbitcoinkernel, which is a great project. However, as it is, can’t be used by Floresta because it relies on core’s block-storage and leveldb. Since floresta is built using utreexo, we can’t need those.

If libbitcoinkernel exposes stateless block/tx validation, that is, I give the transaction[s], the inputs and some context like block hash and MTP, and it spits out a “valid” or “not valid because X”; I would start using it as replacement without problems.

jamesob commented at 7:35 pm on January 5, 2024: member

Concept ACK

If libbitcoinkernel exposes stateless block/tx validation, that is, I give the transaction[s], the inputs and some context like block hash and MTP, and it spits out a “valid” or “not valid because X”; I would start using it as replacement without problems.

Sounds pretty achievable in the short-term, and something that should be exposed for cross-implementation testing anyway?

tcharding commented at 8:45 pm on January 5, 2024: contributor

Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus

Ping @apoelstra @tcharding

rust-bitcoinconsensus can just do a “final” release using v27 and keep existing if folk want to use it, its trivial to maintain.

sanket1729 commented at 0:21 am on January 6, 2024: contributor

concept ACK. cc @danielabrozzoni @notmandatory (BDK group)

This is the list of dependencies that rely on libbitcoinconsensus.

I believe most of them use libbitcoinconsensus for testing script execution, with the only exception of floresta? While I found it beneficial for cross-testing complex scripts, the absence of core policy rule checking became a notable drawback.

We’ve transitioned our projects to utilize the bitcoind crate, incorporating integration tests akin to BitcoinTestFramework in the core. Though the setup might be a bit clunky, I believe(as of now) there is no substitute to running core node to check if a transaction satisfies core policy rules.

apoelstra commented at 4:32 pm on January 6, 2024: contributor

Thanks for the ping. (a) agreed that rust-bitcoinconsensus can just do a “final” release and this wouldn’t be a problem for us, and (b) if there were a blackbox script validator somewhere else, we’d happily use that. Especially if it were a more maintained/supported part of Core rather than something hanging off the side.

jonatack commented at 10:35 pm on January 6, 2024: contributor

ACK a6745f8be306a0aeeb7fb070297ba503321891d0 pending the actions above

Could probably squash. IIUC closes #29123.

DrahtBot requested review from TheCharlatan on Jan 6, 2024

DrahtBot requested review from stickies-v on Jan 6, 2024

DrahtBot requested review from hebasto on Jan 6, 2024

DrahtBot requested review from jamesob on Jan 6, 2024

ajtowns commented at 1:36 am on January 8, 2024: contributor

I believe most of them use libbitcoinconsensus for testing script execution, with the only exception of floresta? While I found it beneficial for cross-testing complex scripts, the absence of core policy rule checking became a notable drawback.

Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?

 0$ ./bitcoin-util -script_flags=STANDARD evalscript '515193'
 1{
 2  "script": {
 3    "asm": "1 1 OP_ADD",
 4    "hex": "515193",
 5    "type": "nonstandard"
 6  },
 7  "sigversion": "witness_v0",
 8  "script_flags": "CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,CLEANSTACK,CONST_SCRIPTCODE,DERSIG,DISCOURAGE_OP_SUCCESS,DISCOURAGE_UPGRADABLE_NOPS,DISCOURAGE_UPGRADABLE_PUBKEYTYPE,DISCOURAGE_UPGRADABLE_TAPROOT_VERSION,DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM,LOW_S,MINIMALDATA,MINIMALIF,NULLDUMMY,NULLFAIL,P2SH,STRICTENC,TAPROOT,WITNESS,WITNESS_PUBKEYTYPE",
 9  "stack-after": [
10    "02"
11  ],
12  "sigop-count": 0,
13  "success": true
14}

BrandonOdiwuor commented at 11:37 am on January 8, 2024: contributor

Concept ACK

apoelstra commented at 2:30 pm on January 8, 2024: contributor

Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?

For my part I would rather have a library function than a CLI tool.

hebasto approved

hebasto commented at 3:18 pm on January 8, 2024: member

ACK a6745f8be306a0aeeb7fb070297ba503321891d0, the diff is correct.

DrahtBot requested review from jonatack on Jan 8, 2024

DrahtBot requested review from ajtowns on Jan 8, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 8, 2024

DrahtBot removed review request from jonatack on Jan 8, 2024

DrahtBot removed review request from BrandonOdiwuor on Jan 8, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 8, 2024

jaonoctus approved

jaonoctus commented at 11:34 pm on January 8, 2024: none

Concept ACK

DrahtBot removed review request from BrandonOdiwuor on Jan 8, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 8, 2024

DrahtBot added the label CI failed on Jan 15, 2024

vasild commented at 11:52 am on January 16, 2024: contributor

Concept ACK on removing the library.

This PR documents the library as deprecated, removes its tests but leaves the library. So, for some time there will be a library that will be untested. What is the merit of doing that? Isn’t it better to document it as deprecated now and later remove the library and its tests at the same time?

DrahtBot removed review request from BrandonOdiwuor on Jan 16, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 16, 2024

brunoerg commented at 1:21 pm on January 16, 2024: contributor

Concept ACK

DrahtBot removed review request from BrandonOdiwuor on Jan 16, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 16, 2024

luke-jr commented at 3:27 am on January 23, 2024: member

Concept NACK, I think? Isn’t this used by libbitcoin and/or btcd??

DrahtBot removed review request from BrandonOdiwuor on Jan 23, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 23, 2024

ajtowns commented at 9:04 am on January 23, 2024: contributor

btcd

libbitcoin optionally uses https://github.com/libbitcoin/libbitcoin-consensus which has a similar name, but doesn’t seem to have any code in common. AFAIK, btcd reimplements all the consensus logic in go.

DrahtBot removed review request from BrandonOdiwuor on Jan 23, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 23, 2024

hebasto referenced this in commit 3659fca1e0 on Jan 25, 2024

achow101 commented at 11:49 pm on January 26, 2024: member

Could you clarify what deprecation will actually mean for 27.0, and then the future steps?

From what I understand. this PR removes a bunch of tests, and adds a sentence to a doc saying that it is deprecated. I think that means we will still be building the library by default but not testing it? That seems a bit odd to me, and I would rather that testing is deleted at the same time the library is removed, unless there is some reason that those tests can be removed that is unrelated to the deprecation.

If we do deprecate this, then all downstream users need to be made aware that it is going away and that they have a plan to move away from it. I don’t like leaving our users out to dry.

For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial. We could also disable building the library for release builds so that those who do not read the release notes will find out that the library was deprecated. As it will still be available in the source, they can still compile it themselves if they need it.

DrahtBot removed review request from BrandonOdiwuor on Jan 26, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 26, 2024

ajtowns commented at 1:49 pm on January 27, 2024: contributor

Could you clarify what deprecation will actually mean for 27.0, and then the future steps?

I would have thought that for 27.x you’d just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0…

For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial. We could also disable building the library for release builds so that those who do not read the release notes will find out that the library was deprecated. As it will still be available in the source, they can still compile it themselves if they need it.

:+1:

DrahtBot removed review request from BrandonOdiwuor on Jan 27, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 27, 2024

theuni commented at 4:33 pm on January 30, 2024: member

@achow101/ @ajtowns Looking again, I agree the order of operations here probably looks a little strange.

For context, most of this discussion came out of the discussion about porting this to CMake. We went in circles for months trying to decide how to best port this over to CMake, but we eventually got together and asked ourselves: should we even bother? Does anyone actually use this?

This PR was our way of fact-finding. It’s a pretty resounding “no”. The lib has been around since 2014 and it’s pretty clear that it’s just not that useful. It can check tx scripts statically, but without a stateful UTXO set there’s very little utility in that. Libbitcoinkernel, which is currently in progress (and which bitcoin-chainstate demonstrates), is already capable of doing much more interesting things, it just doesn’t have a defined api yet.

This PR intended to serve 3 functions:

Gauge any ongoing need (if any) to maintain the lib
Deprecate it and prepare for removal
Remove the bits that are getting in the way of the CMake migration

It seems there’s a general (not 100%, but there never is) agreement on the first and second: it’s ok to deprecate now and remove for v28.

For the third, it probably looks like we’ve jumped the gun a bit. The idea was that these tests really aren’t testing anything interesting at all. They’re just generally covering checks that are done elsewhere, but wrapped around the libbitcoinconsensus apis.

I concede that removing the tests for a lib that we’re still shipping is not ideal. This came more from thinking about the CMake transtion than anything else, but considering that we don’t believe that the tests are actually interesting, it seemed reasonable enough.

Rather than spending time arguing about removing some silly tests, I think there’s a compromise here: Let’s do the deprecation now (the readme changes as well as a release note as suggested by @achow101 above) and plan to just do the lib removal first thing after branch-off. That way we have our assurances that we can simply skip the port on the CMake side so it won’t slow anything down there.

Sound good?

DrahtBot removed review request from BrandonOdiwuor on Jan 30, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 30, 2024

ajtowns commented at 4:53 pm on January 30, 2024: contributor

Rather than spending time arguing about removing some silly tests, I think there’s a compromise here: Let’s do the deprecation now (the readme changes as well as a release note as suggested by @achow101 above) and plan to just do the lib removal first thing after branch-off. That way we have our assurances that we can simply skip the port on the CMake side so it won’t slow anything down there.

Sound good?

Yeah, that’s exactly what I was expecting fwiw.

DrahtBot removed review request from BrandonOdiwuor on Jan 30, 2024

DrahtBot requested review from BrandonOdiwuor on Jan 30, 2024

theuni force-pushed on Jan 30, 2024

libconsensus: deprecate

This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).

Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.

See for example the discussions:
https://github.com/hebasto/bitcoin/pull/41
https://github.com/bitcoin/bitcoin/pull/29123

Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.

25dc87e6f8

theuni force-pushed on Jan 30, 2024

theuni commented at 11:20 pm on January 30, 2024: member

Yeah, that’s exactly what I was expecting fwiw.

Done.

DrahtBot removed the label CI failed on Jan 31, 2024

maflcko added this to the milestone 27.0 on Jan 31, 2024

TheCharlatan approved

TheCharlatan commented at 2:31 pm on February 1, 2024: contributor

ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183

DrahtBot removed review request from BrandonOdiwuor on Feb 1, 2024

DrahtBot requested review from brunoerg on Feb 1, 2024

DrahtBot requested review from jonatack on Feb 1, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 1, 2024

DrahtBot requested review from vasild on Feb 1, 2024

DrahtBot requested review from jaonoctus on Feb 1, 2024

DrahtBot requested review from hebasto on Feb 1, 2024

DrahtBot removed review request from BrandonOdiwuor on Feb 1, 2024

DrahtBot removed review request from jaonoctus on Feb 1, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 1, 2024

DrahtBot requested review from jaonoctus on Feb 1, 2024

fanquake approved

fanquake commented at 4:08 pm on February 1, 2024: member

ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183 - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.

DrahtBot removed review request from BrandonOdiwuor on Feb 1, 2024

DrahtBot removed review request from jaonoctus on Feb 1, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 1, 2024

DrahtBot requested review from jaonoctus on Feb 1, 2024

fanquake merged this on Feb 1, 2024

fanquake closed this on Feb 1, 2024

achow101 commented at 6:42 pm on February 1, 2024: member

Post merge ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183

Sjors commented at 1:10 pm on February 5, 2024: member

This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).

What is the status of LIBBITCOIN_CRYPTO_BASE? Is it kept around or moved into libbitcoin_common?

Sjors commented at 1:12 pm on February 5, 2024: member

I see it’s mentioned here: #29180 (comment) - would be useful to also add a note to libraries.md

Kixunil commented at 4:50 pm on February 15, 2024: none

Hey, I had a use case: create a chain of transactions in WASM (browser) in a multiparty protocol where the child transaction must be verified before signing their parents. The inputs of the parent are controlled by the verifying user so there’s no “you need a full node anyway” argument. Thus libbitcoinkernel is not the solution. Now in my case the template is simple enough to not need libbitcoinconsensus - it’s fine to just verify the signatures. But being able to just run the same code as Core would be very reassuring that there isn’t an attack vector where the counterparty could produce invalid transaction that the client thinks is valid and I wonder if there is any other project that would need more complex verification.

There’s another similar idea I had a long time ago: when ordering a node-in-a-box one could pre-open the channels during payment in the browser (same transaction) so one doesn’t need to wait for channel opens after the node arrives and saves fees. However I’m not familiar with LN inner workings enough to tell whether libbitcoinconsensus would be needed for this. If the LN people could clarify this it’d be nice.

Also please note that I find the announcement period super short. This wasn’t loudly communicated and if I wasn’t a co-maintainer of rust-bitcoin I wouldn’t have known. And even then I’m already writing this post-merge. I’d really appreciate if there was more time given and louder announcements for breaking changes in the future.

Sjors commented at 10:20 am on February 16, 2024: member

Also please note that I find the announcement period super short.

Removal happens at some unknown time after deprecation, but from what I can tell, not before the 28.0 release (~November 2024). The upcoming v27.* releases will be maintained for a while longer too: https://bitcoincore.org/en/lifecycle/

the child transaction must be verified before signing their parents

What exactly do you mean by “verified”? Are you checking proof-of-work from the headers? The signature?

I’m not familiar enough with what functions the kernel library exposes, to know if you can already do that. Otherwise perhaps more functions should be exposed, in order to be able to perform a subset of validation.

fanquake commented at 1:43 pm on February 16, 2024: member

Removal happens at some unknown time after deprecation,

No, removal is going to happen pretty immediately after the 27.x branch off.

Kixunil commented at 2:37 pm on February 16, 2024: none

Removal happens at some unknown time after deprecation

That doesn’t help people who really needed the library. Yeah, they will get screwed later but still they are getting screwed. I meant period to voice opposition to this change. Unless you’re open to un-deprecating of course.

What exactly do you mean by “verified”?

It must be possible to include it in a block at some later time when the parent is confirmed and time locks are satisfied. Basically the same requirement as LN force close transaction just different conditions in scripts.

what functions the kernel library exposes

The information I found is that it performs IO and launches threads - both of those are impossible in WASM, so no functions can help unless it can conditionally not compile syscall-performing stuff. (Though even if it can I expect it will be a hell to integrate into other languages properly.)

maflcko commented at 10:30 am on February 28, 2024: member

That doesn’t help people who really needed the library. Yeah, they will get screwed later but still they are getting screwed.

Is there a reason you can’t stay on the lib of the 27.x or 26.x release?

fanquake referenced this in commit 891a37360e on Mar 13, 2024

fanquake referenced this in commit 87b94949e1 on Mar 13, 2024

luke-jr commented at 9:39 pm on March 14, 2024: member

27.x obviously won’t be supported indefinitely

maflcko commented at 6:57 am on March 15, 2024: member

27.x obviously won’t be supported indefinitely

Why not? I don’t recall the last reported bug in libconsensus, but even if there was one in the future, the 27.x branch could be kept around idle, so that unlikely bugfixes could be applied.

RFC: Deprecate libconsensus #29189

Code Coverage

Reviews

Conflicts