Refactor: Validation: Make parts of IsValidPeginWitness templated #370
Conversation
jtimon
force-pushed
the
e14-pegin-refactor-template
branch
2 times, most recently
from
c515668 to
16ad95c
Compare
jtimon
force-pushed
the
e14-pegin-refactor-template
branch
from
16ad95c to
1d31ac6
Compare
| bool GetAmountFromParentChainPegin(CAmount& amount, const Sidechain::Bitcoin::CTransaction& txBTC, unsigned int nOut) | ||
| { | ||
| amount = txBTC.vout[nOut].nValue; | ||
| return true; |
There was a problem hiding this comment.
just return the value? It seem this can't fail.
There was a problem hiding this comment.
The point is to make the interface like 23af084#diff-24efdb00bfbe56b140fb006b562cc70bR2369 which can fail.
|
utACK 20f9fb3 I'd like another set of eyes on this as well. |
src/validation.cpp
Outdated
| static bool GetBlockAndTxFromMerkleBlock(uint256& block_hash, uint256& tx_hash, T& merkle_block, const std::vector<unsigned char>& merkle_block_raw) | ||
| { | ||
| try { | ||
| std::vector<uint256> txHashes; |
There was a problem hiding this comment.
might as well re-name the variables to match style in here since we're moving them
| return false; | ||
| } | ||
| // TODO do this before to prevent DoS ? | ||
| if (!CheckBitcoinProof(block_hash, merkle_block.header.nBits)) { |
There was a problem hiding this comment.
Checks previous are quite cheap. Not worried.
this should not change behavior
jtimon
force-pushed
the
e14-pegin-refactor-template
branch
from
1d31ac6 to
a20986b
Compare
|
Fixed nits |
| return true; | ||
| } | ||
|
|
||
| template<typename T> |
There was a problem hiding this comment.
I'm curious about the use of a template here (vs., say, an inline static function). This isn't a header file and there only appears to be one use of this template/function -- is this a project coding convention or is there some performance/maintenance gain?
There was a problem hiding this comment.
I'm not the biggest fan of adding code for future use into a pull request, it could sit there making no sense for a long time.
There was a problem hiding this comment.
Yeah, I did it only because @instagibbs asked. Perhaps I should have done 2 commits but not 2 PRs
|
re-utACK a20986b |
|
I built and ran all tests (including qa tests and the peggin.py test) with no errors. I made some comments in #362 that are relevant to #370 I think (362 seemed the better place to make my comments). I got to the point where I understood the code if not the higher level logic that makes these changes necessary (I'm still coming to terms with that). All in all: tACK |
|
ACK |
4 participants
This should not change behavior in any way.
Separated out of #362