Page MenuHomePhabricator

Update getrawtransaction to allow retrieval by including blockid; useful for pruning nodes that cannot use -txindex.
Needs RevisionPublic

Authored by lannocc on Aug 13 2018, 21:08.

Details

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

This patch brings in the ability for pruning nodes to use getrawtransaction by including a blockhash, which has recently been included in bitcoin (https://github.com/bitcoin/bitcoin/pull/10275). Since pruning nodes cannot use the -txindex option, now as long as they know the blockhash which contains the transaction they're looking for (and it hasn't been pruned) then they can retrieve the raw transaction.

Test Plan

Enable pruning or do not use -txindex and then use the getrawtransaction RPC with all three parameters.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
getrawtransaction-pruned-node-support
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3077
Build 4242: Bitcoin ABC Buildbot (legacy)
Build 4241: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 13 2018, 21:08
schancel added inline comments.
src/validation.cpp
609

Did clang-format change this? Looks unnecessary.

jasonbcox requested changes to this revision.Aug 14 2018, 20:54
jasonbcox added inline comments.
src/rpc/rawtransaction.cpp
241

we're moving towards lowerSnakeCasing for our naming conventions. please use inActiveChain instead.

275

This doesn't look related to the rest of the diff.

src/validation.cpp
609

Yes, this is definitely from clang-format. Please use clang-format v4.0 when linting.

1118

Add a comment for what blockIndex does.

1122

blockIndex doesn't appear to be used. I think a bool flag would be better suited here. something like fSkipMempool or something like that

1127

Add a comment to explain why this block is skipped when blockIndex is set.

This revision now requires changes to proceed.Aug 14 2018, 20:54
lannocc marked 2 inline comments as done.

Ammendments after first review feedback.

This removes the unnecessary in_active_chain flag, adds some additional comments, and fixes a bug where blockIndex was not actually being utilized.

deadalnix requested changes to this revision.Aug 15 2018, 18:37
deadalnix added a subscriber: deadalnix.

This does only port a small portion of the original PR. Notably missing are the tests.

This revision now requires changes to proceed.Aug 15 2018, 18:37

This does only port a small portion of the original PR. Notably missing are the tests.

I'm in the middle of a move at the moment and away from my main PC, but I should be set up again next week and will work on incorporating the test cases then.

src/rpc/rawtransaction.cpp
275

I think you're right. It was in the original patch but I don't see how it's necessary for this change. I'll remove it.

src/validation.cpp
609

Yes, yes it did. It changed formatting in a couple other areas as well (line 1121, 1162). I'm guessing previous submitter(s) ignored the linting? Edit: Actually I understand why the other lines were changed because the additional indenting pushed them over the edge. Still not sure why it changed this one though.

609

I used the v4.0.1 binary that @schancel provided to me.

1122

Good catch. That's actually a mistake, I failed to reference it on line 1123.

1123

This is supposed to reference blockIndex now instead of nullptr.