Page MenuHomePhabricator

[RPC] Serialize in getblock without cs_main
Changes PlannedPublic

Authored by IntegralTeam on May 20 2019, 10:23.

Details

Reviewers
deadalnix
markblundeberg
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Summary

Backport PR12151 https://github.com/bitcoin/bitcoin/pull/12151/commits
54dc13b rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures
343b98c rpc: Specify chain tip instead of chain in GetDifficulty
b9f226b rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON
Backport PR15932 https://github.com/bitcoin/bitcoin/pull/15932/commits
fab00a5 rpc: Serialize in getblock without cs_main
faea564 rpc: Add lock annotations to block{,header}ToJSON

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
rpc_block_nolock
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5899
Build 9858: Bitcoin ABC Buildbot (legacy)
Build 9857: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 20 2019, 10:23

Hey, just a heads up that this partly overlaps with D3021. Also when you're backporting things from Bitcoin Core, please make sure to mark them as backports and mention the PR number in the description.

Hopefully D3021 / grandparent D3014 can get merged one day and then let's merge these backports too.

deadalnix requested changes to this revision.May 20 2019, 17:23

Please mention the PR that is backport in the description like this:

  • This is backport of Core PR12345
  • The test plan is inadequate as it doesn't run the RPC tests - and probably should do so using the debug build to check that there are at least no regression (that build is not green for RPC tests).
  • This seems to be a duplicate of D3021 and suffer from the same problem that there is a test dependency that is missing - or if it is not, then there should be at least some justification as to why this is the case.
This revision now requires changes to proceed.May 20 2019, 17:23

@IntegralTeam OK, now my diff is landed -- please rebase onto master, and edit the description to say that this is a backport like so:

PR15932 backport https://github.com/bitcoin/bitcoin/pull/15932/files

As Amaury says, please also add test_runner.py to the test plan.

rebase onto master, and edit the description

IntegralTeam edited the test plan for this revision. (Show Details)

rebase onto master, and edit the description

Now that PR12151 is already backported in a prior diff (D3021) you should take it out of the description, and just focus this on PR15932. :-)

markblundeberg added inline comments.
src/rest.cpp
197 ↗(On Diff #8845)

leave out this change

src/rpc/blockchain.cpp
812 ↗(On Diff #8845)

Original PR brings in RPCHelpMan here and IsValidNumArgs below. If we take this partial backport are we sure to remember to introduce these later on?

907 ↗(On Diff #8845)

The original PR uses GetBlockChecked() instead of these two if-blocks... it looks like the two ways are equivalent but out-of-order backporting makes it tricky to review. Is it not easily possible to backport the priors first?

src/rpc/blockchain.h
12 ↗(On Diff #8845)

stdint.h -> cstdint

This revision now requires changes to proceed.May 24 2019, 14:10
IntegralTeam added inline comments.
src/rpc/blockchain.cpp
812 ↗(On Diff #8845)

@markblundeberg I've look at this functions, it's include some kind of PR's. As a minimum next PR's:

This changes are simple but affect on a much part of files. Would be better create another PR for it.