Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Serialize in getblock without cs_main #15932

Merged
merged 3 commits into from May 3, 2019

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented May 1, 2019

No need to hold cs_main when serializing a struct to json

Fixes: #15925

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1905-rpcBlockNoLock branch from fa59893 to fab00a5 May 1, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Does it make sense to add an AssertLockNotHeld(cs_main) to those functions?

@promag

This comment has been minimized.

Copy link
Member

commented May 2, 2019

For reference #12153 (comment).

Concept ACK.

Does it make sense to add an AssertLockNotHeld(cs_main) to those functions?

Maybe EXCLUDES annotation instead?

@laanwj

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Concept ACK. The whole point of moving the lock of cs_main inside the individual RPC functions was to allow optimizations such as this.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Added lock annotations as requested by @promag and @jnewbery

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14802 (rpc: faster getblockstats using BlockUndo data by FelixWeis)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 2, 2019

utACK faea564

@fanatid

This comment has been minimized.

Copy link

commented May 3, 2019

Why not use ENTER_CRITICAL_SECTION & LEAVE_CRITICAL_SECTION instead LOCK?

@promag

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Because a scoped lock is more clear and "secure".

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 3, 2019

utACK faea564

@MarcoFalke MarcoFalke merged commit faea564 into bitcoin:master May 3, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

MarcoFalke added a commit that referenced this pull request May 3, 2019

Merge #15932: rpc: Serialize in getblock without cs_main
faea564 rpc: Add lock annotations to block{,header}ToJSON (MarcoFalke)
fab00a5 rpc: Serialize in getblock without cs_main (MarcoFalke)
fa1c359 rpc: Use IsValidNumArgs in getblock (MarcoFalke)

Pull request description:

  No need to hold cs_main when serializing a struct to json

  Fixes: #15925

ACKs for commit faea56:
  jnewbery:
    utACK faea564
  jonasschnelli:
    utACK faea564

Tree-SHA512: 005d378cda1e6024e9f5142f99a8adbefe202cd7bfeaafee55eb909e8990a3790aa27fcf5dd16119cc9afe9dc8bd30f660de40233316781669be166bac3018e7

@MarcoFalke MarcoFalke deleted the MarcoFalke:1905-rpcBlockNoLock branch May 3, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 3, 2019

Merge bitcoin#15932: rpc: Serialize in getblock without cs_main
faea564 rpc: Add lock annotations to block{,header}ToJSON (MarcoFalke)
fab00a5 rpc: Serialize in getblock without cs_main (MarcoFalke)
fa1c359 rpc: Use IsValidNumArgs in getblock (MarcoFalke)

Pull request description:

  No need to hold cs_main when serializing a struct to json

  Fixes: bitcoin#15925

ACKs for commit faea56:
  jnewbery:
    utACK faea564
  jonasschnelli:
    utACK faea564

Tree-SHA512: 005d378cda1e6024e9f5142f99a8adbefe202cd7bfeaafee55eb909e8990a3790aa27fcf5dd16119cc9afe9dc8bd30f660de40233316781669be166bac3018e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.