Skip to content

Conversation

hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Sep 6, 2016

Fixes #8125

GetTotalSize() returns the total transaction size (including witness) in
bytes.
@sipa
Copy link
Member

sipa commented Sep 6, 2016 via email

@luke-jr
Copy link
Member

luke-jr commented Sep 6, 2016

I suggest size + feerate-based-on-weight.

@hsjoberg
Copy link
Contributor Author

hsjoberg commented Sep 6, 2016

@sipa Valid point, I guess I could add that as well.
I'm not sure how to make this easy to understand for the user though.

@fanquake fanquake added the GUI label Sep 7, 2016
@jonasschnelli
Copy link
Contributor

I agree with @luke-jr: size + ferrate-based-on-weight

@sipa
Copy link
Member

sipa commented Sep 7, 2016

Which size? We have:

  • Base size
  • Total size (base + witness)
  • Virtual size (base + witness/4)
  • Weight (4*base + witness)

My assumption was that after segwit, only vsize would matter. For old
transactions it is identical to total and base size, and for others it
remains proportional to fees with that.

@jonasschnelli
Copy link
Contributor

Depends what users will do with the size information. But IMO size should be the "total size" (base + witness). I guess the size has usefulness besides fee calculation.

@sipa
Copy link
Member

sipa commented Sep 7, 2016

I agree it may be useful, but total size isn't the first a user should see

  • it's not relevant for fee calculation, propagation time, priority, or
    block space usage.

@jonasschnelli
Copy link
Contributor

I agree that the virtual size does make more sense to display. We just need to clearly distinct between size (concrete hard size of a data structure) and virtual size (including other variables and attached to a bigger context).

I'm also not opposed to display multiple size types (at least base, total and virtual).

@laanwj
Copy link
Member

laanwj commented Sep 8, 2016

Which size? We have:

  • Base size
  • Total size (base + witness)
  • Virtual size (base + witness/4)
  • Weight (4*base + witness)

I think the "easy to implement" tag was a bit deceptive in this case :) #8125 had no discussion about what size at all, I think everyone assumed it to be the size of gettransaction hex.

anyhow, utACK: Let's call it "Total size" and let people add other sizes later if they want.

@hsjoberg
Copy link
Contributor Author

hsjoberg commented Sep 8, 2016

@laanwj Indeed, what seemed like an easy task blew up completely.
I can continue adding other types of sizes if we decide which ones to add.

anyhow, utACK: Let's call it "Total size" and let people add other sizes later if they want.

Will fix.

@maflcko
Copy link
Member

maflcko commented Sep 9, 2016

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@laanwj
Copy link
Member

laanwj commented Sep 13, 2016

@MarcoFalke These are two logically separate commits, one adds the function, the other adds the information to the transaction description HTML, I don't think squashing is necessary.

ACK after the 'size'→ 'total size' disambiguation.

@fanquake
Copy link
Member

It's pretty self explanatory, but here are some OS X screenshots.
transaction
transaction2

@maflcko
Copy link
Member

maflcko commented Sep 16, 2016

utACK c015634

Copy link
Contributor

@paveljanik paveljanik left a comment

Choose a reason for hiding this comment

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

Please squash.

ACK c015634

@jonasschnelli jonasschnelli merged commit c015634 into bitcoin:master Sep 20, 2016
jonasschnelli added a commit that referenced this pull request Sep 20, 2016
c015634 qt: Adding transaction size to transaction details window (Hampus Sjöberg)
 \-- merge fix for s/size/total size/
fdf82fb Adding method GetTotalSize() to CTransaction (Hampus Sjöberg)
@jonasschnelli
Copy link
Contributor

Tested ACK c015634, merge-fixed size->total size.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
GetTotalSize() returns the total transaction size (including witness) in
bytes.

Github-Pull: bitcoin#8672
Rebased-From: fdf82fb
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@hsjoberg hsjoberg deleted the qttxsizeindetails branch March 28, 2017 14:39
codablock pushed a commit to codablock/dash that referenced this pull request Sep 7, 2017
…window

c015634 qt: Adding transaction size to transaction details window (Hampus Sjöberg)
 \-- merge fix for s/size/total size/
fdf82fb Adding method GetTotalSize() to CTransaction (Hampus Sjöberg)
UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Sep 9, 2017
* Merge bitcoin#7506: Use CCoinControl selection in CWallet::FundTransaction

d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)

* Merge bitcoin#7732: [Qt] Debug window: replace "Build date" with "Datadir"

fc737d1 [Qt] remove unused formatBuildDate method (Jonas Schnelli)
4856f1d [Qt] Debug window: replace "Build date" with "Datadir" (Jonas Schnelli)

* Merge bitcoin#7707: [RPC][QT] UI support for abandoned transactions

8efed3b [Qt] Support for abandoned/abandoning transactions (Jonas Schnelli)

* Merge bitcoin#7688: List solvability in listunspent output and improve help

c3932b3 List solvability in listunspent output and improve help (Pieter Wuille)

* Merge bitcoin#8006: Qt: Add option to disable the system tray icon

8b0e497 Qt: Add option to hide the system tray icon (Tyler Hardin)

* Merge bitcoin#8073: qt: askpassphrasedialog: Clear pass fields on accept

02ce2a3 qt: askpassphrasedialog: Clear pass fields on accept (Pavel Vasin)

* Merge bitcoin#8231: [Qt] fix a bug where the SplashScreen will not be hidden during startup

b3e1348 [Qt] fix a bug where the SplashScreen will not be hidden during startup (Jonas Schnelli)

* Merge bitcoin#8257: Do not ask a UI question from bitcoind

1acf1db Do not ask a UI question from bitcoind (Pieter Wuille)

* Merge bitcoin#8463: [qt] Remove Priority from coincontrol dialog

fa8dd78 [qt] Remove Priority from coincontrol dialog (MarcoFalke)

* Merge bitcoin#8678: [Qt][CoinControl] fix UI bug that could result in paying unexpected fee

0480293 [Qt][CoinControl] fix UI bug that could result in paying unexpected fee (Jonas Schnelli)

* Merge bitcoin#8672: Qt: Show transaction size in transaction details window

c015634 qt: Adding transaction size to transaction details window (Hampus Sjöberg)
 \-- merge fix for s/size/total size/
fdf82fb Adding method GetTotalSize() to CTransaction (Hampus Sjöberg)

* Merge bitcoin#8371: [Qt] Add out-of-sync modal info layer

08827df [Qt] modalinfolayer: removed unused comments, renamed signal, code style overhaul (Jonas Schnelli)
d8b062e [Qt] only update "amount of blocks left" when the header chain is in-sync (Jonas Schnelli)
e3245b4 [Qt] add out-of-sync modal info layer (Jonas Schnelli)
e47052f [Qt] ClientModel add method to get the height of the header chain (Jonas Schnelli)
a001f18 [Qt] Always pass the numBlocksChanged signal for headers tip changed (Jonas Schnelli)
bd44a04 [Qt] make Out-Of-Sync warning icon clickable (Jonas Schnelli)
0904c3c [Refactor] refactor function that forms human readable text out of a timeoffset (Jonas Schnelli)

* Merge bitcoin#8805: Trivial: Grammar and capitalization

c9ce17b Trivial: Grammar and capitalization (Derek Miller)

* Merge bitcoin#8885: gui: fix ban from qt console

cb78c60 gui: fix ban from qt console (Cory Fields)

* Merge bitcoin#8821: [qt] sync-overlay: Don't block during reindex

fa85e86 [qt] sync-overlay: Don't show estimated number of headers left (MarcoFalke)
faa4de2 [qt] sync-overlay: Don't block during reindex (MarcoFalke)

* Support themes for new transaction_abandoned icon

* Fix constructor call to COutput

* Merge bitcoin#7842: RPC: do not print minping time in getpeerinfo when no ping received yet

62a6486 RPC: do not print ping info in getpeerinfo when no ping received yet, fix help (Pavel Janík)

* Merge bitcoin#8918: Qt: Add "Copy URI" to payment request context menu

21f5a63 Qt: Add "Copy URI" to payment request context menu (Luke Dashjr)

* Merge bitcoin#8925: qt: Display minimum ping in debug window.

1724a40 Display minimum ping in debug window. (R E Broadley)

* Merge bitcoin#8972: [Qt] make warnings label selectable (jonasschnelli)

ef0c9ee [Qt] make warnings label selectable (Jonas Schnelli)

* Make background of warning icon transparent in modaloverlay

* Merge bitcoin#9088: Reduce ambiguity of warning message

77cbbd9 Make warning message about wallet balance possibly being incorrect less ambiguous. (R E Broadley)

* Replace Bitcoin with Dash in modal overlay

* Remove clicked signals from labelWalletStatus and labelTransactionsStatus

As both are really just labels, clicking on those is not possible.
This is different in Bitcoin, where these labels are actually buttons.

* Pull out modaloverlay show/hide into it's own if/else block and switch to time based check

Also don't use masternodeSync.IsBlockchainSynced() for now as it won't
report the blockchain being synced before the first block (or other MN
data?) arrives. This would otherwise give the impression that sync is
being stuck.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GUI] Show transction size in transaction window.
8 participants