Details

Reviewers

deadalnix
lannocc

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGb07ed750c42e: Use a numeric verbosity with getblock RPC to allow retrieval of the raw…
rABCb07ed750c42e: Use a numeric verbosity with getblock RPC to allow retrieval of the raw…

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

lannocc created this revision.Sep 11 2018, 07:26

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 11 2018, 07:26

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3264: Diff 4873!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.

In D1762#37047, @jasonbcox wrote:

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.

In D1762#37047, @jasonbcox wrote:

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:

As far as I can tell, this change is fully backwards-compatible to the previous boolean argument.
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.
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.

Update release notes

Harbormaster completed remote builds in B3276: Diff 4889.Sep 12 2018, 09:24

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3277: Diff 4890!Sep 12 2018, 09:30

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

In D1762#37200, @deadalnix wrote:

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).

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.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3313: Diff 4941!Sep 16 2018, 13:49

In D1762#37563, @teamcity wrote:

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

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).

deadalnix accepted this revision.Sep 17 2018, 08:32

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

In D1762#37565, @lannocc wrote:

In D1762#37563, @teamcity wrote:

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

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 commandeered this revision.Oct 29 2018, 16:42

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

Rebase

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3703: Diff 5562.Oct 29 2018, 16:57

jasonbcox edited the summary of this revision. (Show Details)Oct 29 2018, 21:19

Closed by commit rABCb07ed750c42e: Use a numeric verbosity with getblock RPC to allow retrieval of the raw… (authored by lannocc, committed by jasonbcox). · Explain WhyOct 29 2018, 21:20

This revision was automatically updated to reflect the committed changes.