Claim name returned is strange #172

Closed

mirgee wants to merge 286 commits from issue-119 into master

mirgee commented

2018-07-13 15:12:04 +02:00

(Migrated from github.com)

Fixing issue #119 .

In bitcoin, small stack pushes are treated as integers when converting scripts to asm. For our use case, it is more sensible to print all pushes as hex strings.

Using tx hex string from the issue thread, tested with

./lbrycrd-cli decodescript b504636174734cdc080110011a7808011230080410011a084d616361726f6e6922002a003214416c6c207269676874732072657365727665642e38004a0052005a001a42080110011a30add80aaf02559ba09853636a0658c42b727cb5bb4ba8acedb4b7fe656065a47a31878dbf9912135ddb9e13806cc1479d220a696d6167652f6a7065672a5c080110031a404180cc0fa4d3839ee29cca866baed25fafb43fca1eb3b608ee889d351d3573d042c7b83e2e643db0d8e062a04e6e9ae6b90540a2f95fe28638d0f18af4361a1c2214f73de93f4299fb32c32f949e02198a8e91101abd6d7576a914be16e4b0f9bd8f6d47d02b3a887049c36d3b84cb88ac

which gives

{
  "asm": "OP_CLAIM_NAME 63617473 080110011a780801123...",
   ...
}

and this is the expected behavior, since 63617473 is hex for cats.

I will follow with further testing or unit tests if necessary.

Fixing issue #119 . In bitcoin, small stack pushes are treated as integers when converting scripts to asm. For our use case, it is more sensible to print all pushes as hex strings. Using tx hex string from the issue thread, tested with ```./lbrycrd-cli decodescript b504636174734cdc080110011a7808011230080410011a084d616361726f6e6922002a003214416c6c207269676874732072657365727665642e38004a0052005a001a42080110011a30add80aaf02559ba09853636a0658c42b727cb5bb4ba8acedb4b7fe656065a47a31878dbf9912135ddb9e13806cc1479d220a696d6167652f6a7065672a5c080110031a404180cc0fa4d3839ee29cca866baed25fafb43fca1eb3b608ee889d351d3573d042c7b83e2e643db0d8e062a04e6e9ae6b90540a2f95fe28638d0f18af4361a1c2214f73de93f4299fb32c32f949e02198a8e91101abd6d7576a914be16e4b0f9bd8f6d47d02b3a887049c36d3b84cb88ac``` which gives ``` { "asm": "OP_CLAIM_NAME 63617473 080110011a780801123...", ... } ``` and this is the expected behavior, since `63617473` is hex for `cats`. I will follow with further testing or unit tests if necessary.

BrannonKing (Migrated from github.com) reviewed 2018-07-13 15:12:04 +02:00

kaykurokawa (Migrated from github.com) reviewed 2018-07-13 15:12:04 +02:00

BrannonKing commented

2018-07-17 21:33:57 +02:00

(Migrated from github.com)

Are you able to add a unit test for this scenario?

lbrynaut commented

2018-07-17 21:56:53 +02:00

(Migrated from github.com)

@BrannonKing Has it been determined that this "fix" is needed?

kaykurokawa commented

2018-07-25 21:44:06 +02:00

(Migrated from github.com)

I was thinking that perhaps this might be too broad of a fix, and it would be better to detect if a script contains a claim OP code and handle it differently, rather than applying this fix to all scripts. Some application might depend on this particular behavior for decoderawtransaction and we might end up breaking it.. although I don't know of any application that does.

lbrynaut commented

2018-07-26 20:00:08 +02:00

(Migrated from github.com)

@kaykurokawa Sounds reasonable. I think we should revisit this after upstream changes because 1) It's not urgent, and 2) Arguably it's not broken (although perhaps not easily understood).

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

BrannonKing

kaykurokawa

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbrycrd#172

No description provided.