test: Activate segwit in TestChain100Setup #19775

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2008-testSegwit changing 2 files +17 −31

MarcoFalke commented at 1:19 PM on August 21, 2020: member
This fixes not only a TODO in the code, but also prevents a never ending source of uninitialized reads. E.g.
- #18376
- #19704 (comment)
- ...
fanquake added the label Tests on Aug 21, 2020

in src/test/util/setup_common.cpp:217 in fad336e02b outdated

 226 |      CBlock& block = pblocktemplate->block;
 227 |  
 228 | -    // Replace mempool-selected txns with just coinbase plus passed-in txns:
 229 | -    block.vtx.resize(1);
 230 | +    Assert(block.vtx.size() == 1);
 231 |      for (const CMutableTransaction& tx : txns)

jnewbery commented at 4:27 PM on August 21, 2020:

since you're touching lines above and below this, you may as well change it to current style in the first commit.

MarcoFalke commented at 4:46 PM on August 21, 2020:

thx, fixed

in src/test/util/setup_common.cpp:214 in fad336e02b outdated

 221 |  {
 222 |      const CChainParams& chainparams = Params();
 223 | -    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(*m_node.mempool, chainparams).CreateNewBlock(scriptPubKey);
 224 | +    CTxMemPool empty_pool;
 225 | +    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(empty_pool, chainparams).CreateNewBlock(scriptPubKey);
 226 |      CBlock& block = pblocktemplate->block;

jnewbery commented at 4:31 PM on August 21, 2020:

I don't think there's any point in this local reference, since the function returns a CBlock value. You could just have:

CBlock block = pblocktemplate->block;

or maybe even better:

    CBlock block = BlockAssembler(empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;

MarcoFalke commented at 4:46 PM on August 21, 2020:

thx, fixed

jnewbery commented at 4:33 PM on August 21, 2020: member

utACK fad336e02b516790b030bc8c474d9995e57fbdc2

Thanks for doing this, @MarcoFalke .

A couple of suggestions to clean up the function a bit more if you feel like it.

test: Move doxygen comment to header

Also, unrelated formatting fixups.

Can be reviewed with --word-diff-regex=.

fa96574b0d

test: Pass empty tx pool to block assembler fa11ff2980
test: Activate segwit in TestChain100Setup fad84b7e14
MarcoFalke force-pushed on Aug 21, 2020
jnewbery commented at 5:47 PM on August 21, 2020: member

utACK fad84b7e14ff92465bc17bfdaf1362bcffe092f6
DrahtBot commented at 1:40 AM on August 26, 2020: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19806 (validation: UTXO snapshot activation by jamesob)
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.
kallewoof commented at 1:58 AM on August 26, 2020: member

Concept and code review ACK
fanquake merged this on Aug 26, 2020
fanquake closed this on Aug 26, 2020
MarcoFalke deleted the branch on Aug 26, 2020
sidhujag referenced this in commit 914f824016 on Aug 26, 2020
Fabcien referenced this in commit f7b6498de8 on Sep 16, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18376 net: fix use-after-free in tests #19704 Net processing: move ProcessMessage() to PeerLogicValidation

test: Activate segwit in TestChain100Setup #19775

Conflicts