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

Adds transaction example #937

Merged
merged 8 commits into from
May 17, 2019
Merged

Adds transaction example #937

merged 8 commits into from
May 17, 2019

Conversation

s-ben
Copy link
Contributor

@s-ben s-ben commented May 10, 2019

Adds an example transaction to the Transaction Details page ( Closes #765 ).

Note: the transaction fields were broken down according to the example tx provided in #765 by @davecgh, and when parsing the data I ended up moving a couple bytes around. E.g. script version in the example was 0000, but the docs say this is a two byte field, so have used 00 and moved the subsequent 00 into the next field (script length). There are also a couple fields that need double checking, such as public key signature length (hex evaluating to 25, though looks like the script has 50 bytes?) and signature script length.

Also, the markdown table is a bit wonky, due to the fact that markdown doesn't support spanning cells. Can tweak the formatting with inline html, but wanted to get a first pass review on the current table first. We could also just use text formatting like we do for the block header example.

Also, do we want to paste a link to the full decoded tx JSON?
https://explorer.dcrdata.org/api/tx/decoded/a58389bd42520a9b9ecbae5432e27b91fde74d2f8e74ac6f8c22cda57b8ae348?indent=true

@davecgh
Copy link
Member

davecgh commented May 10, 2019

Each hex digit is a nibble (4 bits), so 2 bytes takes 4 hex digits. The version is indeed 0000.

Along the same lines, regarding the public key signature length 25 bytes is 50 hex digits.

@s-ben
Copy link
Contributor Author

s-ben commented May 10, 2019

So how is the verision = 1 in the decoded JSON from the explorer,

{
   "txid": "a58389bd42520a9b9ecbae5432e27b91fde74d2f8e74ac6f8c22cda57b8ae348",
   "version": 1,

I believe this was my reasoning/rationalization for my guesswork.

@davecgh
Copy link
Member

davecgh commented May 10, 2019

That's the transaction version. The script version is a different version.

@davecgh
Copy link
Member

davecgh commented May 11, 2019

A couple of things:

  • In the transaction format section, each script should be preceded with a script length field which is itself a variable number of bytes
  • The values are all raw hex values and, as such, imo, shouldn't have additional commas which I find confusing

@s-ben
Copy link
Contributor Author

s-ben commented May 11, 2019

In the transaction format section, each script should be preceded with a script length field which is itself a variable number of bytes

Added variable to table:
Signature script length | The length of the signature script as a variable-length integer

The values are all raw hex values and, as such, imo, shouldn't have additional commas which I find confusing

I found that confusing as well, have removed. Was just following the convention in the block header example. Do we want to get rid of the commas there too?

@s-ben
Copy link
Contributor Author

s-ben commented May 11, 2019

@davecgh with the changes made per above comment, I think we're close to a workable example. However, wondering if more can/should be documented here. Personally, I'd like to understand Decred's transactions at a lower level. E.g. how the variable length integers are parsed. I think this could be valuable to other developers as well. But I'm wondering what questions we're getting from developers specifically, and what are they trying to do generally. Presumably they need to be able to a) parse transactions and b) construct transactions. But what types of transactions? At what level of abstraction? Do we need to document stakebase and other types of transactions? What is Decred-specific, and what is already covered by the Bitcoin dev docs transaction guide? Also, do we want to do a diagram, like this one from the Bitcoin dev guide (which is an open source SVG if we want to start with this)?

image

@davecgh
Copy link
Member

davecgh commented May 11, 2019

I found that confusing as well, have removed. Was just following the convention in the block header example. Do we want to get rid of the commas there too?

Commas are acceptable between each byte (2 hex digits), but there was something strange about how they were formatted such that is was non-standard and confusing.

@davecgh
Copy link
Member

davecgh commented May 11, 2019

An SVG diagram would be helpful I think. As far as the other questions, all transactions follow the format specified here. Votes, Ticket Purchases, etc, are merely specific incantations of them.

@s-ben
Copy link
Contributor Author

s-ben commented May 13, 2019

I believe this (public domain) diagram from the Bitcoin wiki could be easily modified for our purposes,
https://en.bitcoin.it/wiki/File:TxBinaryMap.png

I am wondering if this text from the diagram is true for Decred as well?

Scripts and DER encoding both use big-endian values, all other serializations use little-endian

Also, this may be totally enough for this page. However, when I pretend to be a developer looking at this, I still have questions. What are the rules for encoding different types of transactions? What is the byte map for specific types of scripts? Are they the same as Bitcoin, or are they different? Can I just use Bitcoin documentation for this? What types of scripts are supported? What is the byte map for Decred-specific transactions (e.g. transactions for buying, voting, and revoking tickets)? Do they follow some rules? Do I just need to look to the source code for answers to these questions?

@davecgh
Copy link
Member

davecgh commented May 13, 2019

The bit about script and DER encoding using big-endian values is correct. Regarding the rest, that is way too much to put into a general transaction specification imo. What you're discussing is separate sections which discuss the various types of standard transactions and scripts.

@s-ben
Copy link
Contributor Author

s-ben commented May 13, 2019

The bit about script and DER encoding using big-endian values is correct.

Tried and failed to come up with a simple way to word this. Realized that this is already painstakingly documented for Bitcoin transactions, and that if we're not doing anything different (at this level of abstraction anyway), it's probably not worth re-documenting here.

Regarding the rest, that is way too much to put into a general transaction specification imo. What you're discussing is separate sections which discuss the various types of standard transactions and scripts.

For this page, you're probably right. I do still think it's useful to provide more documentation/explanation of Decred-specific transactions (e.g. stakebase transactions). But that doesn't need to be this page.

Perhaps for this PR, we should just stop here and do a final round of review on what we have so far?

  • Updating Transaction Format section with Signature script length variable
  • Adding transaction example

@davecgh
Copy link
Member

davecgh commented May 13, 2019

I agree that it would be useful to document each one for sure, but I just don't think it belongs in the general transaction specification. They are strictly subsets of generalized transactions.

@davecgh
Copy link
Member

davecgh commented May 13, 2019

In terms of review, the section at the bottom doesn't appear formatted correctly.

@s-ben
Copy link
Contributor Author

s-ben commented May 13, 2019

Agreed, we probably don't want to put any more detail in the general transaction specification. Have proposed documenting Decred-specific transactions further in an Issue to gather feedback on the idea.

Table formatting is fixed. Markdown formatting is limited (e.g. no merged cells) so had to convert to html to get something more readable.

@davecgh
Copy link
Member

davecgh commented May 14, 2019

Already discussed on matrix, but the table formatting is still not correct on some browsers such as Firefox.

@s-ben
Copy link
Contributor Author

s-ben commented May 15, 2019

Ok fixed table formatting for Firefox. Turns out this is a common problem, which gets into some thorny CSS issues. I ended up just hard-coding the text wrapping for cells with long strings; the other, more elegant solutions from SO weren't working. Is this hacky CSS OK? Have tested on Brave, Chrome, Firefox and Safari and rendering fine on all now.

@davecgh
Copy link
Member

davecgh commented May 15, 2019

Still looks broken to me in FF 66.0.5.

docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
docs/advanced/transaction-details.md Outdated Show resolved Hide resolved
@s-ben
Copy link
Contributor Author

s-ben commented May 15, 2019

@davecgh have implemented all your suggested changes, as well as tweaked the formatting to accommodate some of the new long strings (working on FF as well). My only outstanding question is on the Signature Script description (in comment on suggestion).

@davecgh
Copy link
Member

davecgh commented May 15, 2019

I still see several unaddressed review items. They are collapsed.

@s-ben
Copy link
Contributor Author

s-ben commented May 16, 2019

I still see several unaddressed review items. They are collapsed.

Are there wrongly resolved comments? Have addressed the outstanding comments I can see.

@davecgh
Copy link
Member

davecgh commented May 16, 2019

Reviewed updates, resolved relevant comments, and answered questions.

If you look, there is a "7 hidden conversations" in the middle where the review comments are hidden until you click the "Load more...". Those are the unaddressed items I was referring to.

@s-ben
Copy link
Contributor Author

s-ben commented May 16, 2019

Found the "7 hidden conversations" in the middle, but when I click on them it just expands to show already resolved changes :/ Do you see any unresolved changes still on your end? I hate this UI feature.

@davecgh
Copy link
Member

davecgh commented May 16, 2019

I resolved them because you addressed them in the latest commit. I approved the PR already.

@s-ben
Copy link
Contributor Author

s-ben commented May 16, 2019

Cool. Thx for the thorough review/patience on this one 👍

@dajohi dajohi merged commit ebf4ff1 into decred:master May 17, 2019
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.

Add example transaction to Transaction Details page
3 participants