Page MenuHomePhabricator

Use a numeric verbosity with getblock RPC to allow retrieval of the raw transactions
ClosedPublic

Authored by jasonbcox on Sep 11 2018, 07:26.

Details

Summary

This patch changes the getblock RPC verbose boolean argument into a numeric argument called verbosity. The argument now accepts a third option that includes the full raw transactions. Boolean values are still accepted for backwards compatibility, and the default behavior when the argument is omitted remains the same. The possible options for verbosity are: 0 or false for hex-encoded, 1 or true (default) for JSON, and 2 for JSON including the full raw transactions. This patch is simply a port of https://github.com/bitcoin/bitcoin/commit/e3c9f2ddb1e49f719cc1f3f7cc2a98626bee4364.

Backport of Core PR8704

Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>

Test Plan

Call the getblock RPC with a blockhash and 0, 1, 2, true, false for the verbosity argument. Call it without the verbosity argument to check default behavior.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
getblock-verbosity-numeric
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3277
Build 4640: Bitcoin ABC Buildbot (legacy)
Build 4639: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Sep 11 2018, 07:26

This appears to nearly duplicate D1520. I'm willing to abandon that one in favor of this if the diff can be completed, but please make sure to take the old feedback into account.

This appears to nearly duplicate D1520. I'm willing to abandon that one in favor of this if the diff can be completed, but please make sure to take the old feedback into account.

Didn't realize this had already been attempted... taking a look at that now.

Do you know why the staging build failed? I can't find the problem, it compiles fine on my end.

please make sure to take the old feedback into account.

I'll make a note in release-notes.md for the change, but I am confused by your other comments there:

  1. As far as I can tell, this change is fully backwards-compatible to the previous boolean argument.
  2. Regarding the "mixed type input", it could be argued that this is no different than the way integers are treated as booleans in C when context requires it.
  3. I'm confused how you envision having both a verbose and verbosity argument. This seems more naturally a single tri-state switch to me, unless you anticipate also adding raw transactions to the hex-encoded output in the future.

As coded, this brings getblock RPC feature and syntax parity with bitcoin/bitcoin repository, something I think users would appreciate but I'm not sure if that's something Bitcoin-ABC considers generally useful.

The previous commits got lost in the last arc diff... try again.

It looks like from the error log from CI that this is not based on master.

It looks like from the error log from CI that this is not based on master.

This work took multiple commits so I had to run arc diff with "--base git:444640f4396769e3ce2d0ecb0d3a888b47bb2e01" argument so it pulls in the whole range. That base commit is at the top of master from when I created my branch so I'm not sure what the problem is, but I'm very new to the arc workflow. Is this a showstopper? Any suggestions on what I can do?

The CI build complains about BDB 4.8 . We moved to BDB 5.3 a long time ago. Can you try to pull master from phabricator, rebase and rerun arc diff (this will update the diff).

Looking at the pull request here: https://github.com/bitcoin/bitcoin/pull/8704/files it seems like the changes to src/rpc/server.cpp were not ported over. If they are in your branch, please squash then all (You can use git rebase -i to do so).

deadalnix requested changes to this revision.Sep 14 2018, 12:38

Back to your queue.

This revision now requires changes to proceed.Sep 14 2018, 12:38

Bring in the missing changes to server.cpp and rebase to latest master and squash commits.

I've ensured I rebased to the absolute latest from the bitcoin-abc master and squashed it to a single commit. If it's still getting a BDB version error I'm fairly confident the problem is not on my end (another user in the telegram was also having this problem).

This revision is now accepted and ready to land.Sep 17 2018, 08:32

I've ensured I rebased to the absolute latest from the bitcoin-abc master and squashed it to a single commit. If it's still getting a BDB version error I'm fairly confident the problem is not on my end (another user in the telegram was also having this problem).

Based on the error, I'm fairly certain it's because you have not configured arc properly or either not setup your keys with Phabricator. Please double check them and try again.

jasonbcox edited reviewers, added: lannocc; removed: jasonbcox.

Commandeering this diff since it's green and has not been landed for a while.

This revision was automatically updated to reflect the committed changes.