paritytech / parity-bitcoin

The Parity Bitcoin client
GNU General Public License v3.0
730 stars 215 forks source link

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

Open TheBlueMatt opened 7 years ago

TheBlueMatt commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

TheBlueMatt commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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?

jbg commented 7 years ago

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 commented 7 years ago

@jbg That looks interesting, thank you!

5chdn commented 7 years ago

Parity Bitcoin does not plan to use libbitcoin-consensus.

TheBlueMatt commented 7 years ago

@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 commented 7 years ago

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

5chdn commented 7 years ago

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

5chdn commented 7 years ago

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

Kixunil commented 7 years ago

@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.)