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

Additional support for libbitcoin-consensus at compile-time. #404

Open
TheBlueMatt opened this issue Apr 27, 2017 · 19 comments
Open

Additional support for libbitcoin-consensus at compile-time. #404

TheBlueMatt opened this issue Apr 27, 2017 · 19 comments
Labels
F8-enhancement An additional feature request. M4-core Core client code / Rust. P9-somedaymaybe Issue might be worth doing eventually.

Comments

@TheBlueMatt
Copy link

It looks like the script interpreter was written from scratch. This has historically been a major place where consensus bugs are introduced. Bitcoin Core has a "libbitcoinconsensus" library which provides a full script interpreter which is guaranteed to be consensus-compatible with the rest of the Bitcoin network (after all, its the same code). Given the level of review and stability of that part of the code, the safety advantages of Rust arent worth the massive risk of maintaining consensus compatibility, so it might be nice to just swap out the current script interpreter.

@sfultong
Copy link

This has historically been a major place where consensus bugs are introduced.

I think this is precisely why we need multiple implementations. When the protocol evolves in the future, having multiple implementations will help illuminate bugs faster.

That said, having the option for using libbitcoinconsensus in the Parity client would be great.

@roconnor-blockstream
Copy link

roconnor-blockstream commented Apr 27, 2017

find_and_delete looks like it is broken to me. It doesn't seem to parse the opcodes.

@TheBlueMatt
Copy link
Author

I think this is precisely why we need multiple implementations. When the protocol evolves in the future, having multiple implementations will help illuminate bugs faster.

I'm not sure what your point is here? Certainly the consensus logic of Bitcoin is never going to evolve at a rate where the consensus changes aren't tested by future users prior to them being shipped, let alone activated, on the Bitcoin network. Adding additional consensus implementations only slows that down and adds additional potential for bugs, with almost no ability to illuminate issues until they're deployed and there's a billion-dollar bug bounty on exploiting differences (or, at least, we have yet to see any real evidence of that through all the various consensus changes that have been proposed/implemented in the Bitcoin ecosystem).

If it were the case that multiple implementations we're formally proven to be equivalent (by implementing the same API and using static analysis tools) then I might have a very different view (as in that case the static analysis proving equivalence would illuminate differences in implementation prior to shipping which would likely point out sharp edges which might not be ideal API cases), but doing so is fundamentally rather difficult thanks to the pesky halting problem, and I'm not aware of anyone who has tried to do so.

@TheBlueMatt
Copy link
Author

TheBlueMatt commented Apr 27, 2017

@sfultong ahh! that was the confusion. No, my comment about consensus bugs was not between-versions-of-Bitcoin-Core but consensus-bugs-in-other-implementations.

@Kixunil
Copy link

Kixunil commented Apr 28, 2017

Even if Rust implementation of consensus algorithm is interesting, I agree that it should be purely optional and considered experimental.

I can also imagine having both implementations, while libbitcoinconsensus would determine "right from wrong" and if Rust implementation disagrees, issue a warning.

@pradeeproark
Copy link

pradeeproark commented Apr 29, 2017

A rust implementation of libbitcoinconsensus would be great. Any breaking changes between existing libbitcoinconsensus and the rust implementation should be identified by test suites.

@Kixunil
Copy link

Kixunil commented Apr 29, 2017

@PrArk I think either you misunderstood the point of this issue or I misunderstood your comment.

Currently it is implemented in pure Rust. However that's a problem. *Not because of it being Rust but because of it being different from what everybody else runs. For it to be safe, it would have to be absolutely perfectly identical. That's impossible without formally proving equivalence. Tests are not enough because there is always a possibility that someone finds a case in which they differ before someone else does. Single mistake and people can be defrauded.

As much as I'd love to see pure Rust implementation, consensus is more important than code purity.

@pradeeproark
Copy link

@Kixunil, no you did understand me correctly. I know the issues if there are differences in consensus implementations. However, having exactly one reference implementation is not a great way to go about it.

That's impossible without formally proving equivalence. Tests are not enough because there is always a possibility that someone finds a case in which they differ before someone else does

similar complex financial protocol implementations do get by codifying a spec and using formal proofs/tests to verify different implementations. It might be hard but not impossible.

@TheBlueMatt
Copy link
Author

similar complex financial protocol implementations do get by codifying a spec and using formal proofs/tests to verify different implementations. It might be hard but not impossible.

I'm curious which system you're referring to. Ethereum's EVM had no-edge-cases as a clear goal when designing their EVM, and yet major services have had issues and now are forced to run all major nodes and, if they disagree, pause operations. Bitcoin is even worse from an edge-case point of view, and we've seen some of the largest businesses in the Bitcoin space fall on this sword, investing heavily with really smart, capable engineers, only to roll back to using Bitcoin Core border nodes with their software running in SPV mode due to repeated downtime as they get exploited.

For clarify, I dont believe anyone is suggesting that there should or must be only implementation of an entire Bitcoin full node, and the verification of consensus rules part of Bitcoin Core is a relatively small amount of code compared to all the rest. Pulling it out into a library and using that with a healthy ecosystem of P2P/wallet/etc implementations is an order of magnitude simpler than formally proving equivalence, though that is also an option.

@Kixunil
Copy link

Kixunil commented May 1, 2017

Pulling it out into a library and using that with a healthy ecosystem of P2P/wallet/etc implementations is an order of magnitude simpler than formally proving equivalence, though that is also an option.

This is exactly what I had in mind.

As Peter Todd suggested on Reddit, other way to migrate C++ -> Rust would be via hard fork (probably integrated with some other change, like block size increase). I'd be very interested in that but that's probably a long way to go.

@stefment
Copy link

stefment commented May 2, 2017

I think this is precisely why we need multiple implementations. When the protocol evolves in the future, having multiple implementations will help illuminate bugs faster.

Illuminate bugs or cause bugs?

@5chdn 5chdn added the Z1-question Issue is a question. Closer should answer. label May 2, 2017
@jbg
Copy link

jbg commented May 21, 2017

I don't know if this is any use, but I just made https://github.com/jbg/bitcoin-consensus for a project of my own.

@Kixunil
Copy link

Kixunil commented May 22, 2017

@jbg That looks interesting, thank you!

@5chdn
Copy link
Contributor

5chdn commented Aug 17, 2017

Parity Bitcoin does not plan to use libbitcoin-consensus.

@5chdn 5chdn closed this as completed Aug 17, 2017
@TheBlueMatt
Copy link
Author

@5chdn I'd be interested in knowing why. Is there something that you'd like from the libbitcoinconsensus library API that would make it easier to use? I'd be happy to work on making the library easier for consumers.

There is obviously a lot of value in having a diversity of P2P implementations and I was very excited about the possibility of having a Bitcoin-P2P implementation in Rust, but without using libbitcoinconsensus or some other formal function-equivalence-verification tools, the risks of consensus incompatibility are much too great to ever feel comfortable using pairty-bitcoin :(.

@Kixunil
Copy link

Kixunil commented Aug 18, 2017

@5chdn What about a PR that would make it possible to switch between implementations?

@5chdn
Copy link
Contributor

5chdn commented Aug 18, 2017

Sure, contributions are welcome. This should be an optional compile-time feature, however, and not alter existing interfaces significantly.

@5chdn 5chdn reopened this Aug 18, 2017
@5chdn
Copy link
Contributor

5chdn commented Aug 18, 2017

Let's keep this open, if you are seriously interested to keep track of the progress.

@Kixunil
Copy link

Kixunil commented Aug 18, 2017

@5chdn Thank you! I'm not able to start work on this right now, but I guess I will in few months. I was thinking about making it compile-time, but run-time might be useful too (emergency switch in case of bug).

Another possibility: run both and test they return same results. (The result from libbitcoinconsensus would be considered the correct one.)

@5chdn 5chdn changed the title libbitcoinconsensus Additional support for libbitcoin-consensus at compile-time. Aug 21, 2017
@5chdn 5chdn added F8-enhancement An additional feature request. M4-core Core client code / Rust. P9-somedaymaybe Issue might be worth doing eventually. and removed Z1-question Issue is a question. Closer should answer. labels Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F8-enhancement An additional feature request. M4-core Core client code / Rust. P9-somedaymaybe Issue might be worth doing eventually.
Projects
None yet
Development

No branches or pull requests

8 participants