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

Bitcoin matching transactions refactor #2098

Merged
merged 5 commits into from Feb 28, 2020

Conversation

@da-kami
Copy link
Contributor

da-kami commented Feb 26, 2020

WIP - If you are in the mood @thomaseizinger feel free to have a glance - 10% :)

Fixes: #1878

da-kami added 2 commits Feb 26, 2020
…ted, not cleaned up yet
… to be done, TransactionPatterns to be removed, tests to be adapted
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Feb 26, 2020

Are you sure the changelog does not need updating?

Copy link
Contributor

tcharding left a comment

Looking good man! I like the way you used pattern.matches inside the closure to separate the steps of development, nice process.

I made a comment about the abstraction level, if its not clear happy to chat on a call about it.

let transaction = matching_transaction(connector, pattern, start_of_swap)
.await
.context("failed to find transaction to redeem from htlc")?;
let transaction = watch_for_transaction(

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 26, 2020

Contributor

How about adding a layer of abstraction on top of this function, like what we do in ethereum with watch_for_contract_creation -> matching_transaction_receipt. That way the unlock_script (const) would not need to be known to htlc_events and the function could return the secret directly.

This comment has been minimized.

Copy link
@da-kami

da-kami Feb 26, 2020

Author Contributor

I implemented it the same way as for Ethereum - the secret is extracted in ethereum/htlc_events.rs, not in watch_for_event. I actually like it that way - extracting the secret is a different concern than finding the transaction/event for me.

This comment has been minimized.

Copy link
@da-kami

da-kami Feb 27, 2020

Author Contributor

Discussed this further with Thomas and understand better what you meant Tobin.
Happy to change the criteria for distinguishing between refund and redeem in a different PR (change from the 0 or 1 flag to use the redeem or refund pubkey. However, I would not distinguish the refund and redeem cases in different functions (this would be required if we want to hide the criteria).

Copy link
Member

thomaseizinger left a comment

Good stuff so far, you are heading the right direction! :)

start_of_swap: NaiveDateTime,
from_outpoint: OutPoint,
unlock_script: Vec<Vec<u8>>,

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

Thinking ahead for miniscript, we might want to change this to be less generic and only provide a public key as the "spending with" condition.

i.e. we want to match whether it was spent with the redeem or the refund public key. To do that, I think we just have to check for the presence of the public key on the witness stack.

You can do this in a follow-up PR though if you want to keep the scope small.


let (vout, _txout) = transaction
.find_output(&compute_address)
.expect("Deployment transaction must contain outpoint described in pattern");

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

It would be good to completely eliminate this error path but for that, you will need to change the API of matching_transaction to return you the Outpoint right away.

/// watching).
///
/// It yields those blocks as part of the process.
async fn find_relevant_blocks<C>(

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

I can already see us sharing this code between Bitcoin and Ethereum, nice :)

type Hash = bitcoin::BlockHash;
type Block = bitcoin::Block;

// TODO: reevaluate naming of function

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

watch_for_outpoint_spent?


let mut prev_blockhashes: HashSet<bitcoin::BlockHash> = HashSet::new();
let mut missing_block_futures: Vec<_> = Vec::new();
// TODO: reevaluate naming of function

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

Could create some nice symmetric with the above suggestion and name this: watch_for_outpoint_created.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 27, 2020

Member

Rationale: Sending bitcoin to an address creates an (unspent) outpoint :)

@tcharding

This comment has been minimized.

Copy link
Contributor

tcharding commented Feb 27, 2020

Nit: This is not a 'refactor', this is a total re-write of the matching logic for Bitcoin. I just stumbled on this looking through PR titles, it may happen in the future if we ever are searching for this.

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Feb 27, 2020

Nit: This is not a 'refactor', this is a total re-write of the matching logic for Bitcoin. I just stumbled on this looking through PR titles, it may happen in the future if we ever are searching for this.

What, if not this is a refactoring?

Hopefully, there is no observable change from the outside of the system :)

@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Feb 28, 2020

bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 28, 2020

@bors bors bot merged commit e8e924d into dev Feb 28, 2020
5 checks passed
5 checks passed
automation
Details
Summary 3 rules match
Details
bors Build succeeded
Details
ci/circleci: debug-build-test Your tests passed on CircleCI!
Details
license/cla All CLA requirements met.
@mergify mergify bot deleted the 1878-bitcoin-matching-transactions-refactor branch Feb 28, 2020
bors bot added a commit that referenced this pull request Mar 2, 2020
Merge #2126
2126: Revert "Bitcoin matching transactions refactor" r=thomaseizinger a=thomaseizinger

Reverts #2098

We are reverting this because it causes e2e test failures.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.