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

Put locktime into FullTxOut #950

Closed
rajarshimaitra opened this issue Apr 25, 2023 · 9 comments
Closed

Put locktime into FullTxOut #950

rajarshimaitra opened this issue Apr 25, 2023 · 9 comments

Comments

@rajarshimaitra
Copy link
Contributor

Describe the enhancement
To calculate maturity for time-locked transactions, the lock time needs to be included in FullTxOut. #926 (comment)

This could be done in together with #945

Use case
Enchance maturity calculation.

@rajarshimaitra rajarshimaitra added new feature New feature or request BDK 1.0.0 and removed new feature New feature or request labels Apr 25, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Apr 27, 2023

Do you mean the lock time for the transaction that the output is on or do you mean time at which this output could be spent?

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Apr 28, 2023

Could be both. Either the locktime fields directly from the transaction the utxo is sitting in, or the final time/height at which it becomes "spendable".

Unlike coinbase, time-locked txs are in mempool. But unlike normal mempool utxos, which are spendable, we should not include them in coinselection. So they need to be filtered out via is_mature or some other function.

As in terms of what data to include. It can in-theory be both, the actual locktime fields from the transaction, or the final unlocking time/height. But both will need knowledge of confirmation heights somewhere to work out maturity.

My personal preference would be towards putting the final unlocking time/height in FullTxOut and handle the calculation in TXGraph::try_list_chain_txouts. In is_mature only check if the height <= tip.

Reasoning: it hides the complex part in TxGraph. Makes maturity function simple. Gives a clear understanding where confirmation height information fits best in TxGraph. makes FullTxOut simpler to interact with.

@rajarshimaitra
Copy link
Contributor Author

Another idea:

Why not just put this final maturity time/height in TxGraph itself for each tracked utxos? For both coinbase and time locked transactions we can do this.

Benefits:

  • This can be done at the sync time while inserting relevant txs in IndexedTXGraph.

  • removes the need of storing confirmation heights in anchor.

  • Clone the same field in FullTxOut and maturity is as simple as height <= tip.

I don't know which one is more complex or simpler. Doing this way is a burden on the user to do this right.

@rajarshimaitra
Copy link
Contributor Author

As per this comment #951 (comment)

We can go with Idea 1. And put the final time/height of unlocking in FullTxOut.

I will make a draft implementation for it.

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Apr 28, 2023

Oh.. We also need to know the block-time from chainoracle at a given height to handle time-based locks. Also need the blocktime at confirmation height to handle time-based relative locks. How to do that?

We can punch in these data into Anchors. But is there any other plan for handling them?

cc: @evanlinjin

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Apr 28, 2023

I made some trials, and it seems we need the following: (More complex than I initially thought)

  • ChainOracle to tell time given a height. This changes the details of LocalChain as well as the ChainOracle trait. We need to blocktime at tips to tell if something by nLocktime matured or not.

  • A new Anchor that stores both confirmation height and time. Probably need a new trait too as the current Anchor don't return time.

  • Ensure the previous TxNodeInternal is stored in the graph even if it's not "relevant". We need it for relative time-locks. But we need to do this in IndexedTxGraph::insert_relevant_tx() and the user needs to ensure they put the previous tx in the relevant txs list too.

Does this seem like a reasonable approach? Or else open for suggestion.

@LLFourn
Copy link
Contributor

LLFourn commented May 1, 2023

Could be both. Either the locktime fields directly from the transaction the utxo is sitting in, or the final time/height at which it becomes "spendable".

  1. We don't always have the full transaction for an output in the graph. We could do a "best effort" locktime for both absolute and relative and put it here. I feel it's not very useful since txs that are not yet valid because of timelock are unlikely to be in the graph at all and so it feels weird to include this information on the txouts. It can be queried by querying the transaction. We could have a bool returning method to tell you whether a tx is valid with respect to lock times at the given height/time.
  2. We don't know when am output becomes spendable because we don't necessarily have the descriptor. I think this kind of filtering needs to be done at another level.

@rajarshimaitra
Copy link
Contributor Author

I agree that this can be done in another layer. Maybe at the wallet level.

While ensuring we include confirmation time for the input transaction too, while doing the sync. It doesn't have to be a full Tx. Could be the Partial Tx like we store in the TxGraph. But it would require another Anchor type with confirmation_time as a parameter.

@LLFourn
Copy link
Contributor

LLFourn commented May 1, 2023

I agree that this can be done in another layer. Maybe at the wallet level.

While ensuring we include confirmation time for the input transaction too, while doing the sync. It doesn't have to be a full Tx. Could be the Partial Tx like we store in the TxGraph. But it would require another Anchor type with confirmation_time as a parameter.

I meant that we can't know what the nlocktime field is of a partial transaction. Closing this for now. We'll have to deal with the concerns brought up here at some point but the tooling does not belong in FullTxOut since that it just some convenience type that we return in some places. Business logic doesn't belong on it I don't think.

@LLFourn LLFourn closed this as completed May 1, 2023
@notmandatory notmandatory added this to BDK May 4, 2023
@notmandatory notmandatory removed this from BDK May 4, 2023
@notmandatory notmandatory added this to BDK May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants