Refactor TxToJSON() and ScriptPubKeyToJSON() #8824
Conversation
jnewbery
changed the title
jnewbery
force-pushed
the
JSON_refactor
branch
from
42d102c to
c3bf285
Compare
|
#8817 has now been merged into master. I've rebased this PR. |
jnewbery
changed the title
|
bitcoin-test-util.py now fails json tests because output includes transaction size and vsize. I'll fix up the tests in a new commit. |
|
Concept ACK. |
src/core_write.cpp
Outdated
There was a problem hiding this comment.
Maybe ::GetVirtualTransactionSize(tx)?
There was a problem hiding this comment.
GetVirtualTransactionSize() is declared in policy.h, which isn't included in core_write.
Why is GetVirtualTransactionSize() in policy? Because transaction virtual size isn't a consensus rule, and is only used for mempool admittance/miner selection. On top of that, after #8365 , virtual size has been overloaded to mean both transaction weight/4 and an adjusted virtual size taking account of sigops cost.
I could update core_write.cpp to include policy, but I don't think it's a good idea to include policy more widely than necessary.
|
@jonasschnelli - are you happy with my response above? Any further questions? |
jnewbery
force-pushed
the
JSON_refactor
branch
2 times, most recently
from
3952ae4 to
22c34e7
Compare
|
rebased. Is anyone happy to ACK this? @jonasschnelli - did you have time to review my response above? |
src/core_write.cpp
Outdated
There was a problem hiding this comment.
Doesn't this require std::max(GetTransactionWeight(tx), nSigOpCost * nBytesPerSigOp)?
Nm: it's not required here.
|
Tested ACK 22c34e7e4c74d2c7d1cc64a75c6654f3ae82377b |
jnewbery
force-pushed
the
JSON_refactor
branch
2 times, most recently
from
e9deba0 to
bf7b745
Compare
|
Rebased |
jnewbery
force-pushed
the
JSON_refactor
branch
from
bf7b745 to
44e6628
Compare
|
Rebased |
src/rpc/server.h
Outdated
There was a problem hiding this comment.
Server is the wrong place for this - the conceptual idea is that it is a purely a RPC server, and independent of anything bitcoin specific.
There was a problem hiding this comment.
Hm, I think you can simply remove the changes to this file as the function is in core_write now?
|
Addressed @laanwj 's review comment and rebased. |
|
Needs a rebase. |
|
Rebased |
|
It looks like this got entangled with a test change? here's a rebased version: https://github.com/laanwj/bitcoin/tree/2017_04_jnewbery_JSON_refactor Was planning to merge it, but the json test changes in a RPC refactoring pull don't look right. |
|
"size" was added to the getrawtransaction output in #7072. "vsize" was added in #8149. Because bitcoin-tx used Sorry that wasn't clear in the OP. |
|
rebased. Last time please :) |
0ff9320 refactor TxToJSON() and ScriptPubKeyToJSON() (jonnynewbs) Tree-SHA512: caf7d590829e221522edd5b1ab8ce67b53a2c6986d3bbe8477eab420b1007bf60f885ed0a25ba9587e468c00768360ddc31db37847e862858573eaed5ed8b0d6
Summary: This PR removes almost all of the TxToJSON() and ScriptPubKeyToJSON() code from bitcoin-rpc and places it in bitcoin-common, removing about 80 lines of duplicate code. The only part that can't be moved into bitcoin-common is parsing block contextual information (confirmations and blocktime), which are not available to bitcoin-common code. Backports Core PR8824 bitcoin/bitcoin#8824 Completes T500. Test Plan: make check Reviewers: jasonbcox, deadalnix, O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity Differential Revision: https://reviews.bitcoinabc.org/D2499
0ff9320 refactor TxToJSON() and ScriptPubKeyToJSON() (jonnynewbs) Tree-SHA512: caf7d590829e221522edd5b1ab8ce67b53a2c6986d3bbe8477eab420b1007bf60f885ed0a25ba9587e468c00768360ddc31db37847e862858573eaed5ed8b0d6
* Merge bitcoin#8824: Refactor TxToJSON() and ScriptPubKeyToJSON() 0ff9320 refactor TxToJSON() and ScriptPubKeyToJSON() (jonnynewbs) Tree-SHA512: caf7d590829e221522edd5b1ab8ce67b53a2c6986d3bbe8477eab420b1007bf60f885ed0a25ba9587e468c00768360ddc31db37847e862858573eaed5ed8b0d6 * remove witness and vsize Signed-off-by: Pasta <[email protected]> * Add valueSat To preserve rpc output format * Move extrapayload and special tx json output to `TxToUniv` * Add spent index info
5 participants
This PR removes almost all of the
TxToJSON()andScriptPubKeyToJSON()code from bitcoin-rpc and places it in bitcoin-common, removing about 80 lines of duplicate code. The only part that can't be moved into bitcoin-common is parsing block contextual information (confirmations and blocktime), which are not available to bitcoin-common code.This PR should be merged along with #8817 , which ensures that witness data gets returned from calls to TxToUniv()