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

fix: Allow minimum work headers for a testnet/simnet #148

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Conversation

vitsalis
Copy link
Member

@vitsalis vitsalis commented Sep 18, 2022

During testing with a local BTC testnet, I discovered that the proof of work checks failed. After some investigation to the functionality that Bitcoin does for the validation of headers, I made a minor update to the validation checks.

A major design decision is the usage of more relaxed tests than the ones that bitcoind implements. This decision was made to reduce complexity. This will lead to users being able to submit headers that would not be accepted by the bitcoind implementation, but this ability is limited by:

  • For the testnet/simnet the work should be at least as the minimum work for the network.
  • For the mainnet, the work should be at least 25% or 400% the work of the parent header. An attacker could use this to submit headers that are 25% less over time, making this rule relax to the minimum work of the network.

Overall, the above mean that an attacker can insert clutter but they must do work for it. Added a very detailed comment that describes the design decisions and what bitcoind does compared to us.

Also, refactored the msgServer implementation to allow tests to work without producing headers that have enough work.

Tested with my local bitcoind testnet and the error did not re-appear.

Useful links:
validation of headers
calculation of the expected Bits field value

we will do all required checks, but we will still not test the exact value of the `Bits` field.
Instead we will verify that the `Bits` field value is going to be in a valid range and if someone
does BTC mainnet work to add clutter on the BBN chain, then we can tolerate that.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comprehensive comments. I will have to read it like 10 more times. Maybe ask a review @fishermanymc as well to understand any security implications.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I will have to go through this a few more times.

Just to make sure I understand:

  • On mainnet, we do take into account that not every parent-child can change the difficulty, only every 2048th block can, right?
  • What was the problem on the testnet exactly?

@vitsalis
Copy link
Member Author

vitsalis commented Sep 19, 2022

Yep, the expected work is recalculated every two weeks: the parameter that defines that can be found here.

On both the mainnet and the testnet, the difficulty can change every two weeks. However, this difficulty change can be only up to 25% or 400% of the previous difficulty (a range between those values). However, on the testnet, it is allowed for headers to have the minimum amount of required work in order to ensure the liveness of the chain. See the comment here.

The design decision that I make in this PR is to check that:

  1. the work that is performed for the block is at least more than the minimum in both the testnet and the mainnet
  2. only for the mainnet, I check that the work of the parent block is in the range of (25%-400%).

For (1), we just ensure that someone has committed at least the minimum work required. The fact that we do not perform check (2) for the testnet means that someone could submit a sequence of headers with the minimum work required for the testnet, introducing clutter. However, the most difficult chain rule will be maintained, so no security issue here, just a storage issue (that will not be that much since the attacker needs to perform work).

For (2), the bitcoin core reference checks whether the Bits field is exactly the same as the value that is expected (and calculated), but here we check that it is in the range of values that are allowed. I made this design decision, because I do not want to mess with the precision of arithmetic operations between the Go language and C++ and to simplify the code. An attacker can take advantage of the fact that we do not check the exact value and that we do not check that only after two weeks and perform an attack that does the following:

  1. Submit a sequence of headers with each one having 25% less work than the previous one.
  2. After some time, this would lead to headers getting accepted with the minimum mainnet difficulty, relaxing the security checks to the one at (1). However, no security issues here, since an honest vigilante would submit real mainnet headers, so the most difficult chain rule will still be maintained.

The issue with the testnet (and leading to the crash) is that it contains minimum work headers because it is allowed, but the current implementation does not allow this.

Overall, I would suggest looking at the code links I specified in the comments, I think they would make this more clear, as well as the validation code that invokes it.

I think we should do something simple for now in order to have the testnet working since I do not see any security issues arising and in the future we can consider adding more complex checks (they would have to be extensively reviewed and verified).

@fishermanymc
Copy link

Is the root issue that BTC testnet has a different difficulty adjustment and verification rule compared to the mainnet?

@vitsalis
Copy link
Member Author

They have the same difficulty adjustment rule. However, the testnet also allows headers that have much different work than their parents, that is up to the minimum amount for work.

Copy link
Collaborator

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes looks good. Thanks for thorough explanation in comments!

In general I would say we should implements header validation as close as possible to bitcoin implementation, but taking into account this statement:

I think we should do something simple for now in order to have the testnet working since I do not see any security issues arising and in the future we can consider adding more complex checks

I assume it will be temporary change (i.e for now) to enable babylon to easy testing on btc testnet. If thats the case, maybe create issue on github and leave todo in code, so we do not forget about it ?

@vitsalis
Copy link
Member Author

Agreed, created an issue, we can track it from there.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough analysis, great stuff!

we do not check that only after two weeks

I think we should check that on the mainnet, because as you say the adversary could create a string of low difficulty bitcoin headers to trigger the k-deep and w-deep rules on forks that don't exist on BTC but were easy to create for them.

I understand this might not be the easiest thing to add so it's fine to be in a separate PR but it seems important.

@vitsalis vitsalis merged commit 57c30c0 into main Sep 20, 2022
@vitsalis vitsalis deleted the btc-header-work branch September 20, 2022 03:56
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.

4 participants