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

WIP: Get rid of depth in TransactionConfidence. #1553

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schildbach
Copy link
Member

This is work on #877. It's very preview, breaks tests and therefore not intended for merging soon.

Issues:

  • Yes, it's an API breaking change. Mostly this means you have to replace confidence.getDepthInBlocks() by confidence.getDepthInBlocks(wallet.getLastBlockSeenHeight());.
  • Height is removed from the wallet protobuf. The field number is blocked for future use. Luckily appearsAtBlockHeight was already present so no migration is needed.
  • As intended, ConfidenceListener will not fire on depth change, since the depth is now not part of the confidence any more. This means wallets with responsive UI will miss blockchain confirmations – unless they subscribe to some of the block height listeners too. Perhaps we should add a wallet-based "lastBlockSeenListener", since I suspect the existing listeners will perhaps not fire at the right time.
  • The TransactionConfidence.getDepthFuture() needs a replacement, based on the above listener.
  • Theoretically, if a block arrives which is irrelevant to the wallet saving could be skipped. Unfortunately, we still need to persist the lastBlockSeenHeight field so for now Wallet.saveLater() still needs to be called on each new block.
  • DefaultCoinSelector spends coins based on priority (depth * value). Since priority is no longer a thing with miners (they pick by fee rate) and depth is now not easily available to the DefaultCoinSelector, I tried changing the algorithm to "older first". Unfortunately this breaks many tests. I guess this decision/work should be broken out to a separate PR.

@micheal-swiggs
Copy link

micheal-swiggs commented Feb 28, 2023

I can see a possible solution to this. It would involve creating a new TxConfidenceTable object maybe ConfidenceManager object that is used by Wallet. Methods in Wallet are the consumers of depth. The only thing I wonder about is if we want to implement the changes as breaking or non-breaking changes? To accomplish non-breaking changes it would require accessing ConfidenceManager via Context from within TxConfidenceTable. The depth related methods of TxConfidenceTable would start making calls to ConfidenceManager via Context. The depth methods within TxConfidenceTable would be deprecated and the new methods on ConfidenceManager should be used instead. It doesn't feel like the most pretty solution but is perhaps justified for one release as the dependency on Context could be removed once the deprecated methods on TxConfidenceTable are removed...

confidence.clearBroadcastBy();
confidenceChanged.put(tx, TransactionConfidence.Listener.ChangeReason.DEPTH);
Copy link

@micheal-swiggs micheal-swiggs Mar 20, 2023

Choose a reason for hiding this comment

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

@schildbach I suppose this is the line that is causing the so called "useless work" referenced in #877 ?

@schildbach schildbach modified the milestones: 0.17, 0.18 Apr 27, 2024
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.

None yet

2 participants