[RPC] Transaction details in getblock #2506

salisbury-espinosa · 2018-11-27T13:20:44Z

No description provided.

UdjinM6 · 2018-11-27T14:56:35Z

This could be controlled by yet another rpc parameter or verbose could become 0-2 instead of bool to provide all options. And this is exactly what was already implemented in upstream, so I'd rather postpone this till future backports.

salisbury-espinosa · 2018-11-28T09:48:06Z

@UdjinM6 And this is exactly what was already implemented in upstream, so I'd rather postpone this till future backports.
taked from bitcoin#8704

UdjinM6

This is incomplete and doesn't match the original PR in couple of places unfortunately. Also, this should be based on top of develop because we do NOT merge into master neither directly nor via PRs. And finally, while this is nice to have and it is not that invasive, I'm still leaning towards postponing these changes till future backports and not including it in 0.13. @codablock

src/rpc/blockchain.cpp

salisbury-espinosa · 2018-11-30T17:25:24Z

This is incomplete and doesn't match the original PR in couple of places unfortunately.

answered in conversation (This line should be dropped. - fixed)

Also, this should be based on top of develop because we do NOT merge into master neither directly nor via PRs.

fixed (checkout develop + cherry pick + push --force)

And finally, while this is nice to have and it is not that invasive, I'm still leaning towards postponing these changes till future backports and not including it in 0.13

if don’t make it, users will have to patch source and build software to get detailed information about transactions. because after block 264000 in testnet, the parsing of raw blocks with https://github.com/bitcoinjs/bitcoinjs-lib stopped working @codablock @UdjinM6

UdjinM6

if don’t make it, users will have to patch source and build software to get detailed information about transactions. because after block 264000 in testnet, the parsing of raw blocks with https://github.com/bitcoinjs/bitcoinjs-lib stopped working

That's interesting. Mind clarifying the problem (error log or anything)? @nmarley , pls note the issue.

Also, see inline comments for requested code changes.

src/rpc/blockchain.cpp

salisbury-espinosa

@UdjinM6 #2506 (comment) #2506 (comment) - fixed

That's interesting. Mind clarifying the problem (error log or anything)?

That's how I get transactions from the block:

import type { Block, Transaction } from "bitcoinjs-lib";
const bitcoin = require("bitcoinjs-lib");
// here logic of fetching hexFromRpc from rpc...
const parsedBlock: Block = bitcoin.Block.fromHex(hexFromRpc);
const txList: Array<PostProcessedTransactionType> = tx: parsedBlock.transactions.map((tx: Transaction): PostProcessedTransactionType => {
      // there is some sort of processing logic
    })

this code was valid before block 264000 (dash-testnet)
now it raises an error:

RangeError [ERR_INDEX_OUT_OF_RANGE]: Index out of range\n    at checkOffset (buffer.js:1015:11)\n    
at Buffer.readUInt32LE (buffer.js:1079:5)\n    
at readUInt32 (/home/blockchains-parser/node_modules/bitcoinjs-lib/src/transaction.js:57:20)\n    
at Function.Transaction.fromBuffer (/home/blockchains-parser/node_modules/bitcoinjs-lib/src/transaction.js:110:17)\n    
at readTransaction (/home/blockchains-parser/node_modules/bitcoinjs-lib/src/block.js:57:26)\n    
at Function.Block.fromBuffer (/home/blockchains-parser/node_modules/bitcoinjs-lib/src/block.js:66:14)\n    
at Function.Block.fromHex (/home/blockchains-parser/node_modules/bitcoinjs-lib/src/block.js:82:16)

because of this, I will have to patch the code and build the blockchain node from release to release until this code is accepted

PS (off topic)
By the way, it would be nice to add some information to the wiki, who uses rpc and is currently parsing raw blocks using javascript (or making your own fork of bitcoinjs-lib for dash):
snippet of getting block hash:

import type { Block } from "bitcoinjs-lib";
const multiHashing = require("multi-hashing-bbscoin");
const getBlockHash = (block: Block, typeChain: ?string): string => {
  if (["dash", "dash-testnet"].indexOf(typeChain) > -1) {
    return multiHashing
      .x11(block.toBuffer(true))
      .reverse()
      .toString("hex");
  }
  return block.getId();
};

src/rpc/blockchain.cpp

UdjinM6 · 2018-12-01T18:44:49Z

So, it seems like you are fixing the wrong problem :)

You should just add DIP2 (https://github.com/dashpay/dips/blob/master/dip-0002.md) support in your bitcoinjs-lib fork, smth like that UdjinM6/bitcoinjs-lib@69807fd

salisbury-espinosa · 2018-12-01T23:50:24Z

So, it seems like you are fixing the wrong problem :)

@UdjinM6 The problem is the impossibility of extracting transaction details from the block.
There are 2 ways to solve the problem

rpc upgrade to get transaction details by block hash
create fork of bitcoinjs-lib with support for all implemented DIP's in dash core

The first method is the most correct, since it relies on the code base where these improvement proposals are implemented ( https://github.com/dashpay/dash ).

For the second method

it is necessary to support 2 forks (one for dash and one for the rest) in the code of the consumer (js parser)
it is necessary to know all the features of the implementation of these DIP's and duplicate protocol specific features in the fork UdjinM6/bitcoinjs-lib@69807fd (and therefore, from release to release, it will probably break and be able to be supported only by the developer who implemented these dip's)

so:

in your bitcoinjs-lib fork

I would not like to have 2 fork in the consumer code (js)

You should just add DIP2

it's easy to do only after you have added DIP2 to js ( UdjinM6/bitcoinjs-lib@69807fd )
:)

PS
https://jenkins.dash.org/blue/organizations/jenkins/dashpay-dash/detail/PR-2506/1/pipeline/#step-219-log-1142

dip3-deterministicmns.py | False | 868 s

very strange for this PR @UdjinM6 @codablock
https://github.com/dashpay/dash/commits/develop/qa/rpc-tests/dip3-deterministicmns.py
but I see it seems that this check fails for a very long time...

UdjinM6 · 2018-12-02T15:42:24Z

The first method is the most correct

Well, that's debatable imo. The output json data is 10-ish times larger than the raw one, so for a full 2mb block you'll have to parse 20mb-ish json. And without patching bitcoinjs-lib you'll have to avoid parsing/producing raw data at all times and would have to only rely on workarounds instead, which throws quite a bit of bitcoinlib-js functionality out of the window imo.

it is necessary to support 2 forks (one for dash and one for the rest)

Not really. The way I implemented it, it should be backwards compatible with bitcoin/litecoin/etc. so you only need just 1 single fork with these features implemented.

it is necessary to know all the features of the implementation of these DIP's and duplicate protocol specific features in the fork UdjinM6/bitcoinjs-lib@69807fd (and therefore, from release to release, it will probably break and be able to be supported only by the developer who implemented these dip's)

That's a good point. Maybe we could maintain such a fork in dashpay or dashevo repo. Thoughts @codablock @nmarley ?

salisbury-espinosa · 2018-12-02T16:20:26Z

Well, that's debatable imo. The output json data is 10-ish times larger than the raw one, so for a full 2mb block you'll have to parse 20mb-ish json.

I think it's faster to parse a 20mb-ish json than to parse a 2mb block hex by javascript (but there are no benchmarks on my hands)
the only problem is to transmit this amount of information (20mb) (network issue etc), but this is not bottleneck - blocks are infrequently produced + this is just a few megabytes in peak for a fairly long period of time

parsing/producing raw data at all times

only when a new block arrives

The good side of this approach is that the logic of obtaining useful data will lie entirely on the code base of dash core, which already contains all the code for work and does not need to be tied to third-party code (bitcoinjs-lib)

The way I implemented it, it should be backwards compatible with bitcoin/litecoin/etc. so you only need just 1 single fork with these features implemented.

UdjinM6/bitcoinjs-lib@69807fd

if (tx.version === 3) {
    tx.extrapayload = readVarSlice()
  }

what if a litecoin or bitcoin transaction appears with version === 3 and they will need another logic?
they will not necessarily execute the protocol by DIP2...
so it will work exactly until other coins start using version == 3 and in order to avoid future errors it is already necessary to make a separate fork and and connect it separately in the code base of the consumer

@UdjinM6

UdjinM6 · 2018-12-03T11:38:17Z

what if a litecoin or bitcoin transaction appears with version === 3 and they will need another logic?

True, good point.

UdjinM6

utACK

codablock

utACK

Add missing versionHex field (dashpay/dash@e7d9ffa) Change to use verbosity syntax (dashpay/dash#2506 and bitcoin/bitcoin#8704)

* Content - RPC - Update quick reference * RPC - Update getblockchaininfo to show BIP-9 progress Related to dashpay/dash#2435 * RPC - Update gobject prepare with new params Use-IS (dashpay/dash#2452) Use specific UTXO for fee (dashpay/dash#2482) * RPC - Update mode name * RPC - Update protx default mode dashpay/dash#2513 * Content - Add spork 17 * Content - Special transactions Add info for Quorum commitment Remove messages not in 13.0 (SubTx) * P2P - Add new txlvote fields masternodeProTxHash (dashpay/dash#2484) quorumModifierHash (dashpay/dash#2505) * RPC - Update protx list Make all options follow the same parameter format (dashpay/dash#2559) * Content - version bump 0.13.0.0 bumped to 70213 (dashpay/dash#2557) * Guide - PrivateSend dstx message limit Up to 5 simultaneous dstxs per MN allowed (dashpay/dash#2552) * RPC - Update getblock Add missing versionHex field (dashpay/dash@e7d9ffa) Change to use verbosity syntax (dashpay/dash#2506 and bitcoin/bitcoin#8704) * P2P - Add qfcommit message (no hexdump example) DIP6 quorum final commitment (dashpay/dash#2477) * P2P - qfcommit typo Change description of llmqType field * P2P - Special tx payload size clarification * Guide - Update MN payment description Related to dashpay/dash#2258 * Guide - fix broken link * Guide - Update some example txs Change to hashes on the chain following the 12.3.4 reset * P2P - Add QcTx hexdump * P2P - DIP4 message updates Add SML entry Update hexdump to include new fields Add getmnlistd and mnlistdiff to cross ref * P2P - minor DIP3-related comments

salisbury-espinosa changed the title ~~added full info about transactions into rpc endpoint getblock~~ [RPC] Transaction details in getblock Nov 28, 2018

salisbury-espinosa changed the base branch from master to develop November 28, 2018 12:28

salisbury-espinosa changed the base branch from develop to master November 28, 2018 12:28

UdjinM6 requested changes Nov 29, 2018

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

salisbury-espinosa force-pushed the master branch from 503a3c8 to afc39b6 Compare November 30, 2018 16:49

salisbury-espinosa changed the base branch from master to develop November 30, 2018 16:53

salisbury-espinosa changed the base branch from develop to master November 30, 2018 17:00

salisbury-espinosa force-pushed the master branch from afc39b6 to a9ef165 Compare November 30, 2018 17:17

salisbury-espinosa changed the base branch from master to develop November 30, 2018 17:17

UdjinM6 requested changes Nov 30, 2018

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Use a verbosity instead of two verbose parameters

36ceb53

salisbury-espinosa force-pushed the master branch from a9ef165 to 36ceb53 Compare December 1, 2018 14:27

salisbury-espinosa commented Dec 1, 2018

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

UdjinM6 requested a review from codablock December 3, 2018 11:38

UdjinM6 added this to the 13.0 milestone Dec 3, 2018

UdjinM6 approved these changes Dec 3, 2018

View reviewed changes

codablock approved these changes Dec 3, 2018

View reviewed changes

codablock merged commit 1270b71 into dashpay:develop Dec 3, 2018

thephez added a commit to dash-docs/dash-docs that referenced this pull request Dec 17, 2018

RPC - Update getblock

b652fe1

Add missing versionHex field (dashpay/dash@e7d9ffa) Change to use verbosity syntax (dashpay/dash#2506 and bitcoin/bitcoin#8704)

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Jun 29, 2020

Use a verbosity instead of two verbose parameters (dashpay#2506)

3511dac

[RPC] Transaction details in getblock #2506

[RPC] Transaction details in getblock #2506

Uh oh!

Conversation

salisbury-espinosa commented Nov 27, 2018

Uh oh!

UdjinM6 commented Nov 27, 2018

Uh oh!

salisbury-espinosa commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

salisbury-espinosa commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

salisbury-espinosa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salisbury-espinosa commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Dec 2, 2018

Uh oh!

salisbury-espinosa commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Dec 3, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

salisbury-espinosa commented Nov 28, 2018 •

edited

Loading

salisbury-espinosa commented Nov 30, 2018 •

edited

Loading

salisbury-espinosa left a comment •

edited

Loading

UdjinM6 commented Dec 1, 2018 •

edited

Loading

salisbury-espinosa commented Dec 1, 2018 •

edited

Loading

salisbury-espinosa commented Dec 2, 2018 •

edited

Loading