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

[VDG] Add Confirmation Time to Transaction Details #11092

Merged

Conversation

SuperJMN
Copy link
Collaborator

As the title says, this adds the estimated estimated confirmation time.

@SuperJMN SuperJMN marked this pull request as ready for review July 23, 2023 15:11
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

I don't see the confirmation time.

image

@SuperJMN
Copy link
Collaborator Author

@yahiheb @turbolay can you please check now? :)

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 23, 2023
@turbolay
Copy link
Collaborator

Better now, but there are still variations between the time displayed in the TX preview screen and this screen, which looks a bit weird. Also, I believe it doesn't handle CPFP etc...

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

There are two issues:

  • The confirmation time is not the same on the Preview Transaction and on the Transaction Details. (As mentioned by turbolay)
  • If the transaction gets confirmed while the Transaction Details dialog is open, the confirmation time section shouldn't be visible.

3

4

SuperJMN and others added 2 commits July 24, 2023 09:23
@SuperJMN
Copy link
Collaborator Author

  • If the transaction gets confirmed while the Transaction Details dialog is open, the confirmation time section shouldn't be visible.

Should be fixed. Good catch!

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

Everything seems to work fine for me, tACK

However, but this PR is not the place I believe:

@SuperJMN SuperJMN requested a review from soosr July 24, 2023 09:59
ichthus1604
ichthus1604 previously approved these changes Jul 24, 2023
Copy link
Collaborator

@ichthus1604 ichthus1604 left a comment

Choose a reason for hiding this comment

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

utACK.

Code is okay.

Cannot test right now since TestNet wallets are working erratically.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

The confirmation time is still not the same on the Preview Transaction and on the Transaction Details, sometimes with big difference sometimes just a bit.

Maybe the issue is with the Preview Transaction's confirmation time and not this PR.

1
2
3
4

@nopara73
Copy link
Contributor

If it takes hours, don't show minutes. If it takes days, don't show hours. If it takes weeks, don't show days, and so on...

Otherwise, we allude accuracy to the user that we don't have. Furthermore, even if we could accurately predict, the user still would not need to be bothered by such noise.

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 25, 2023

Regardless of displaying the minutes, hours, days or not which should be done in a separate PR, the issue here is why do we get different results for the supposedly the same thing? We should get the same result.

@soosr
Copy link
Collaborator

soosr commented Jul 25, 2023

I can reproduce @yahiheb's issue, and the source of the problem is the FeeRate calculation.
During sending I chose 8.413 sat/vbyte and the transaction was created with 8.444 sat/vbyte, while the transaction details received 12.x sat/vbyte

The transaction: https://mempool.space/testnet/tx/a884f63cd6d1726131da60049ff87bb9e97c4dc5312d3fb1a114ddc953ddba74

It works well with selfspend btw.

@SuperJMN
Copy link
Collaborator Author

I still don't see how the calculation can be wrong. I'm just using the Fee and VirtualSize to calculate the FeeRate:

        public FeeRate? FeeRate
	{
		get
		{
			if (Fee is null)
			{
				return null;
			}

			return new FeeRate(Fee, VirtualSize);
		}
	}

I'm lost. Can someone drop me a helping hand? cc @zkSNACKs/code-team

@SuperJMN SuperJMN requested a review from turbolay July 25, 2023 23:25
@turbolay
Copy link
Collaborator

The issue is rare and not caused by this PR.
I could only reproduce it when sending 1 coin from the same address to the same address, such as this TX:
https://mempool.space/testnet/tx/c09935ccb09b3aa17d73badec223e1cca390e182f392824f2d25103b3fcab0a8

I don't have the time right now to investigate more, but the issue is that containingTransaction.Transaction.GetVirtualSize() gives me a VSize of 82 when it should be 110 according to mempool.space (and I assume to the TransactionBuilder as well, but I didn't verify)

Everything seems to work well with this PR by itself, except the CPFP as I already mentioned.
Another thing to note, I assume that the Update Reactive command misses Skip(1) for the TransactionDetails because when I open the window everything is always called twice.

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 26, 2023

@soosr
Copy link
Collaborator

soosr commented Jul 26, 2023

https://mempool.space/testnet/tx/30fe22c646eeae7b500652736566a16faadcb91254589a2bc940eb9d20bf296a The fee rate used: 1.82 sat/vB

Capture1 Capture2

Could you debug what is the fee rate that the transaction details received?

@nopara73
Copy link
Contributor

FTR, I don't know how exactly you guys are estimating the fees, but the fee estimator that you're using only gives you a single entry back:
image

https://wasabiwallet.co/api/v4/btc/Blockchain/all-fees?estimateSmartFeeMode=CONSERVATIVE

This means 1 s/b should confirm within 20 minutes. So it's strange that 1.8 s/b gives you 3 and 8 hours.

@soosr
Copy link
Collaborator

soosr commented Jul 26, 2023

On TestNet it uses hardcoded values

private static readonly Dictionary<int, int> TestNetFeeEstimates = new()
{
[1] = 17,
[2] = 12,
[3] = 9,
[6] = 9,
[18] = 2,
[36] = 2,
[72] = 2,
[144] = 2,
[432] = 1,
[1008] = 1
};

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 26, 2023

Could you debug what is the fee rate that the transaction details received?

It received 2.426 Sat/B while it is 1.82 sat/vB on https://mempool.space/testnet/tx/320bc22297f905498834857c89880319dea1f3d6c0c6e560d988ad62e01d8c5f

image

@nopara73
Copy link
Contributor

AllFeeEstimate now has unit tested fee estimations:

public TimeSpan EstimateConfirmationTime(FeeRate feeRate)
public bool TryEstimateConfirmationTime(SmartTransaction tx, [NotNullWhen(true)] out TimeSpan? confirmationTime)

@SuperJMN @soosr please use this

@SuperJMN
Copy link
Collaborator Author

SuperJMN commented Jul 26, 2023

AllFeeEstimate now has unit tested fee estimations:

public TimeSpan EstimateConfirmationTime(FeeRate feeRate) public bool TryEstimateConfirmationTime(SmartTransaction tx, [NotNullWhen(true)] out TimeSpan? confirmationTime)

@SuperJMN @soosr please use this

@nopara73 please check what I did. I played by ear there.
I've tested it briefly and confirmation times seem coherent.

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 27, 2023

Now the confirmation time is the same, but the time on the fee chart is wrong.

Notice that the fee rate that the Preview Transaction/Details received is 2.426 Sat/B while it is 1.82 sat/vB on mempool.space.
As Turbolay mentioned in his comment:

the issue is that containingTransaction.Transaction.GetVirtualSize() gives me a VSize of 82 when it should be 110 according to mempool.space

What is this VirtualSize 82 compared to 110 vB? and what is 2.426 Sat/B compared to 1.82 sat/vB (different units)?

22
33

11
44

@nopara73
Copy link
Contributor

As promised. Merging. Remaining issues:

  • fee selection chart != conf time (I mean, they are the same, it's just the former is more refined.)
  • conf time visibility update doesn't happen when tx details is open (@SuperJMN it should be easy to fix for you, if you have time, please do it)
<!-- Confirmation Time -->
      <c:PreviewItem IsVisible="{Binding IsConfirmationTimeVisible}"
                     Icon="{StaticResource timer_regular}"
                     Label="Confirmation time"
                     CopyableContent="{Binding ConfirmationTime}">
        <c:PrivacyContentControl>
          <TextBlock Text="{Binding ConfirmationTime, Converter={x:Static conv:TimeSpanConverter.ToEstimatedConfirmationTime}}" Classes="monoSpaced" />
        </c:PrivacyContentControl>
      </c:PreviewItem>

      <Separator IsVisible="{Binding IsConfirmationTimeVisible}" />

@nopara73 nopara73 merged commit 4047080 into zkSNACKs:master Jul 27, 2023
2 of 6 checks passed
@SuperJMN SuperJMN deleted the improvements/tx-details-confirmation-time branch July 27, 2023 07:54
@SuperJMN
Copy link
Collaborator Author

  • conf time visibility update doesn't happen when tx details is open (@SuperJMN it should be easy to fix for you, if you have time, please do it)

Fixed in #11116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants