Github Issue

Background

Like Block.Height, BlockTransaction and BlockOperation can have their own ordered index. The advantage of the indexes is that 1) make readable ordering for DB index and cursor, 2) we can easily realize like that the Second op of 21 tx of 1202 height.. and all nodes have same indexes.

Background: All nodes have the same indexes if nodes have the same hash.

Solution

[X] Add Index field to BlockTransaction and BlockOperation
[X] Add index field to api/resource/BlockTransaction and api/resource/BlockOperation
[x] Test codes
[x] Make DB Keys of BlockTransaction and BlockOperation.
[x] PageCursor
[x] Support prev and next using one more thing....

Possible Drawbacks

Nov 20 '18 05:11 anarcher

$http localhost:2821/api/v1/blocks/162 | jq .transactions
[
  "G6dxdjTcn4DG3F2BXR52udqcnMSx38X2BUECGZFQpXo",
  "Bjb5LXkdSoWWzBb6AJzxkfpv3KtA78LpXkqUxMNxRijz",
  "BbJHTfEvUEppUFq1QZYmZUu5nA3RPd5v1JcBw9K4b9Rs",
  "eXkEV1niwq7d78jvNaVnvAWdggAozFWsScG3VaryHrv",
  "AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5",
  "5T1EVPwK1RoBBBst3SrSLJVaQP6vNpRJuRdWhdVKs57n",
  "4W7j9uEx6sdYewq7qbD2PNFgap39YyVj5szJkx5WeBGn",
  "CNP2Xwh89sUhWCbCfcDmfjF8EgvHeHij3bqWeMFnsAJD",
  "BXmLKMvMbRru53tbmnDNreJbTz77PwXNXjhJnrPm2Cs5",
  "BfvBxeBr142mDYZkjgiBzn8xnS1R8D84zXwHvDPsa1rY"
]
$http localhost:2823/api/v1/transactions/AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5 | jq .index
5
http localhost:2823/api/v1/transactions/AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5/operations | jq ._embedded.records[].index
0

http localhost:2823/api/v1/transactions/AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5
HTTP/1.1 200 OK
Content-Length: 571
Content-Type: application/hal+json
Date: Tue, 20 Nov 2018 05:53:22 GMT
X-Ratelimit-Limit:
X-Ratelimit-Remaining:
X-Ratelimit-Reset:

{
    "_links": {
        "account": {
            "href": "/api/v1/accounts/GBO5VCIB3CRFZIGMMOPFXZ6AY4FTLKW4UK2AW22DIQ6X5XLQDFWBEVGG"
        },
        "operations": {
            "href": "/api/v1/transactions/AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5/operations{?cursor,limit,order}",
            "templated": true
        },
        "self": {
            "href": "/api/v1/transactions"
        }
    },
    "block": "FtZYPHRxbnbRQq1bi1hkbbzGGVW1JK2hwwGi7qJhGYuM",
    "created": "2018-11-20T14:35:27.125297000+09:00",
    "fee": "10000",
    "hash": "AH2D27NvKKUNHeaRFbNq3FBUDt6MnbooNGUkGrH1zrY5",
    "index": 5,
    "operation_count": 1,
    "sequence_id": 39,
    "source": "GBO5VCIB3CRFZIGMMOPFXZ6AY4FTLKW4UK2AW22DIQ6X5XLQDFWBEVGG"
}

Nov 20 '18 05:11 anarcher

Codecov Report

Merging #765 into master will decrease coverage by 0.19%. The diff coverage is 77.77%.

@@           Coverage Diff            @@
##           master    #765     +/-   ##
========================================
- Coverage   60.49%   60.3%   -0.2%     
========================================
  Files         153     156      +3     
  Lines       10708   10859    +151     
========================================
+ Hits         6478    6548     +70     
- Misses       3498    3572     +74     
- Partials      732     739      +7

Flag	Coverage Δ
#integration_tests_long_term	`44.21% <54.67%> (-0.34%)`	:arrow_down:
#integration_tests_node	`40.21% <54.67%> (-0.16%)`	:arrow_down:
#unittests	`48.57% <70.46%> (-0.09%)`	:arrow_down:

Impacted Files	Coverage Δ
lib/node/runner/api/api.go	`62.96% <ø> (+2.24%)`	:arrow_up:
lib/block/test.go	`75.67% <100%> (ø)`	:arrow_up:
lib/common/util.go	`33.73% <100%> (-1.56%)`	:arrow_down:
lib/block/block.go	`48.83% <100%> (ø)`	:arrow_up:
lib/node/runner/block_operations.go	`64.73% <100%> (+1.05%)`	:arrow_up:
lib/node/runner/api/resource/transaction.go	`59.45% <100%> (+2.31%)`	:arrow_up:
lib/block/genesis.go	`79.68% <100%> (ø)`	:arrow_up:
lib/node/runner/api/resource/operation.go	`83.33% <100%> (+0.57%)`	:arrow_up:
lib/node/runner/finish_ballot.go	`43.75% <100%> (ø)`	:arrow_up:
lib/block/order.go	`48% <48%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5c5b761...185a0a6. Read the comment docs.

Nov 20 '18 06:11 codecov-io

As you can see here https://github.com/bosnet/sebak/blob/820e28d9d9d230026889f25b42448dc73d4ed540/lib/block/operation.go#L51-L53

BlockOperation's Hash is a combination of TxHash and OpHash. BecauseOpHash of BlockOperation is not unique. Let's assume that Account1 pay 1000 amount to Account2. And after that, Account1 pay 1000 amount to Account2 again. In this case, the first operation and the second operation is exactly the same and the hashes also the same. Hence, OpHash is not unique.

To overcome this, As-is implementation uses the combination of TxHash and OpHash for unique ID of BlockOperation.

Different from others such as Block and Transaction, Operation's Hash is just an index key for the database.

In conclusion, therefore, if this PR support index key as a combination of the sequence number of Block, Transaction, and Operation, then we can replace the Hash of operation with the index key And the Hash is no longer needed to be exposed to a client by API.

@anarcher What do you think about this?

Nov 20 '18 06:11 kfangw

@kfangw IMHO, the ophash is proof of validating an operation's payload. So I am not entirely sure but Exposing ophash to API is not bad. But The /api/v1/transactions/{txHash}/operations/{opHash} is unique location. Or Supporting /api/v1/transactions/{txHash}/operations/1 ? haha...

With #767 , My original idea is to use Tx.Index and Op.Index for ordering part for cursor (not prefixing part). But I am not sure that we can use it for prefixing part. Such as {height}-{tx.Index}-{op.Index} humm...

Nov 20 '18 08:11 anarcher

@outsideris @kfangw Thanks for kindly discuss. I feel that I understand this opHash issues.

Nov 20 '18 09:11 anarcher

$http localhost:2822/api/v1/transactions limit==100 | jq -c "._embedded.records[] | [ .block , .index ]"
["59EKzLpsAbmJGFT3DmCL9rt6M3WQz8CRhJU7mkejW53N",8]
["59EKzLpsAbmJGFT3DmCL9rt6M3WQz8CRhJU7mkejW53N",9]
["59EKzLpsAbmJGFT3DmCL9rt6M3WQz8CRhJU7mkejW53N",10]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",0]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",1]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",2]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",3]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",4]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",5]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",6]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",7]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",8]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",9]
["2XBxxbhfbwPPoQ6RFDciEKGtGdicmRr4MvxdqztJQcbw",10]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",0]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",1]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",2]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",3]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",4]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",5]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",6]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",7]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",8]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",9]
["DB9d94gywvmTTmnem9ECPYREoGAuEAK9EYc5wZPA6uK",10]
["4aD7QvZsGR4XyXqkafhGJn7wojTrCkHYBbCTCWWLVM6R",0]

Nov 20 '18 12:11 anarcher

$http localhost:2822/api/v1/transactions cursor==20-0 | jq -cr "._embedded.records[] | [ .block_height,.index] "
[20,0]
[21,0]
[22,0]
[23,0]
[24,0]
[25,0]
[26,0]
[27,0]
[28,0]
[29,0]
[30,0]
[31,0]
[32,0]
[33,0]
[34,0]
[35,0]
[36,0]
[37,0]
[38,0]
[39,0]
(go:bos) anarch@chainer ~/go/bos/src/sebak-hot-body (master)
$http localhost:2822/api/v1/transactions cursor==20-0 | jq -cr "._links"
{"next":{"href":"/api/v1/transactions?cursor=39-0&limit=20&reverse=false"},"prev":{"href":"/api/v1/transactions?cursor=39-0&limit=20&reverse=true"},"self":{"href":"/api/v1/transactions?cursor=20-0"}}

What do you think about it? @spikeekips @kfangw @Charleslee522

Nov 21 '18 06:11 anarcher

So far, so good. When you finished, let me know. @anarcher

Nov 21 '18 09:11 kfangw

I almost finished this PR.... 💤

Nov 21 '18 14:11 anarcher

I finished it. plz review it....

Nov 22 '18 01:11 anarcher

Plz review it :-> @spikeekips @Geod24 @Charleslee522 @soonkuk

Nov 22 '18 10:11 anarcher

Plz review it :-> @spikeekips @Geod24 @Charleslee522 @soonkuk

please review it @spikeekips @Geod24 @Charleslee522 @soonkuk it is an urgent issue.

Client developer report an issue that the cursor is not correct. This patch might solve the problem.

Nov 22 '18 13:11 kfangw

ping @spikeekips @Geod24 @Charleslee522 @soonkuk

Nov 23 '18 01:11 kfangw

ping @spikeekips @Geod24 @Charleslee522 @soonkuk

Nov 23 '18 08:11 anarcher

I rebased #763 on the master, not this PR. Hence, this PR no longer urgent.

Nov 23 '18 08:11 kfangw

@anarcher When I test this PR with frozen accounts api, it did not find the frozen accounts.

Nov 27 '18 06:11 soonkuk

@anarcher I tested this again. It was not from this PR. After applying recent change in master, it works fine.

Nov 27 '18 06:11 soonkuk

LGTM!

Nov 27 '18 06:11 soonkuk

I will make a merge commit from master to this. @anarcher

Nov 27 '18 10:11 kfangw

https://github.com/bosnet/sebak/pull/765/commits/20116e17fd9850e72c3b406b5ce7a5447edb7adc

Ping @kfangw @Charleslee522 @spikeekips @Geod24 @soonkuk

I added BlockIndexes... I would like to listen to your opinions about BlockIndexes. :->

Dec 11 '18 08:12 anarcher

20116e1

Ping @kfangw @Charleslee522 @spikeekips @Geod24 @soonkuk

I added BlockIndexes... I would like to listen to your opinions about BlockIndexes. :->

Looks good to me. Is the BlockIndexes generalized to the others? (Transaction, Account, and Operation)

Dec 11 '18 08:12 kfangw

Is the BlockIndexes generalized to the others? (Transaction, Account, and Operation)

I think that TxIndexes and AccountIndexes and OperationIndexes... :->

Dec 11 '18 08:12 anarcher

I revert BlockIndexes.
How do I do about this PR? ㅠ

Dec 18 '18 07:12 anarcher

type Index interface {
    Key() string  //  key = {prefix}/{order}
    Prefix() string // {prefix}/
    Order() string  // order = {prefix}/{order}...  // order is parts of key or key
}

How about this interface?

BlockXXX.Save() and GetXXX() has a relation with index(key,prefix,order). (key and prefix and order). But the current design has not. It makes confusing to read when an object has more indexes

Dec 18 '18 08:12 anarcher

Add BlockTransaction.Index and BlockOperation.Index

Github Issue

Background

Solution

Possible Drawbacks

Codecov Report