Qt: Show transaction size in transaction details window #8672

Merged
merged 2 commits into from Sep 20, 2016

Projects

None yet

8 participants

@Cocosoft
Contributor
Cocosoft commented Sep 6, 2016

Fixes #8125

Cocosoft added some commits Sep 6, 2016
@sipa
Member
sipa commented Sep 6, 2016
@luke-jr
Member
luke-jr commented Sep 6, 2016

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

@Cocosoft
Contributor
Cocosoft 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
Member

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

@sipa
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
Member

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
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
Member

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
Member
laanwj commented Sep 8, 2016 edited

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.

@Cocosoft
Contributor
Cocosoft commented Sep 8, 2016 edited

@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.

@laanwj
Member
laanwj commented Sep 13, 2016 edited

@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
Contributor

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

@MarcoFalke
Member

utACK c015634

@jonasschnelli jonasschnelli merged commit c015634 into bitcoin:master Sep 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasschnelli jonasschnelli added a commit that referenced this pull request Sep 20, 2016
@jonasschnelli jonasschnelli Merge #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)
6052d50
@jonasschnelli
Member

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

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@Cocosoft @luke-jr Cocosoft + luke-jr Adding method GetTotalSize() to CTransaction
GetTotalSize() returns the total transaction size (including witness) in
bytes.

Github-Pull: #8672
Rebased-From: fdf82fb
73cd552
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@Cocosoft @luke-jr Cocosoft + luke-jr qt: Adding transaction size to transaction details window
Github-Pull: #8672
Rebased-From: c015634
98381f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment