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

Add basic DLC Oracle #2094

Merged
merged 8 commits into from Oct 3, 2020
Merged

Conversation

benthecarman
Copy link
Collaborator

@benthecarman benthecarman commented Sep 30, 2020

Closes #2096
Closes #2099

Copy link
Contributor

@Christewart Christewart left a comment

Choose a reason for hiding this comment

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

Also I think @nkohen should review this as it's very sensitive code.

label: String,
outcomes: Vector[String]): Future[EventDb] = {
for {
indexOpt <- rValueDAO.maxKeyIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

So I do worry about the usage of maxKeyIndex. There is nothing that protects against two separate threads calling createNewEvent() at the same time. If I understand correctly, this would lead to an r value being re-used?

I think it would be wiser to have rValueDAO be responsible for retrieving and incrementing the maxKeyIndex. We should then write test cases to make sure we can't get rValueDAO to return the same index on separte calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's not very vulnerable to that. The db has a primary key for the nonce so it should fail if you try to make a second with the same, also the r value is tweaked based on the label so it's extra safe there.


hash =
CryptoUtil.sha256(nonce.bytes ++ CryptoUtil.serializeForHash(label))
commitmentSig = signingKey.schnorrSign(hash.bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is commitmentSig? I was under the assumption it was the final signature published, but it seems that it is just affirming that you actually own the private key? This seems like something that can be replaced if we put signatures on oracle addresses? discreetlogcontracts/dlcspecs#99

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The commitmentSig is the oracle signing their R value to prove that the oracle is infact going to sign for this event / R value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be more clear they aren't signing the R value but instead

CryptoUtil.sha256(nonce.bytes ++ CryptoUtil.serializeForHash(label))

val dummyNonce = SchnorrNonce(ECPublicKey.freshPublicKey.bytes.tail)
recoverToSucceededIf[RuntimeException](
dlcOracle.signEvent(dummyNonce, "testOutcomes"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So I would like to see a test case that calls createEvent() twice in parallel a verify that the same nonce is NEVER returned. In general, we should always make sure the database configuration for the DLC oracle project has numThreads=1 to make sure that there is no concurrent database accesses. We should use a require() inside of DLCOracleAppConfig to enforce this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue filed for the latter half of this issue: #2107

@Christewart Christewart added the database database related work for our various projects label Oct 2, 2020
@Christewart Christewart merged commit 5d56e95 into bitcoin-s:master Oct 3, 2020
@benthecarman benthecarman deleted the dlc-oracle-mod branch October 3, 2020 16:30
Christewart pushed a commit that referenced this pull request May 1, 2021
* Add basic DLC Oracle

* Respond to review

* Respond to more review

* Add maturation time

* Add to testkit, tag hashes, better val names

* More clear vals, version tagged hashes

* Signing key clean up

* Add pubkey to db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database database related work for our various projects oracle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventDb doesn't store public key associated with an event Choosing a signing key for a DLCOracle
3 participants