Skip to content

fuzz: Create a block template in tx_pool targets #21798

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 4 commits into from
May 6, 2021
Merged

fuzz: Create a block template in tx_pool targets #21798

maflcko merged 4 commits into from
May 6, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 28, 2021

Relatively simple check to ensure a block can always be created from the mempool

MarcoFalke added 3 commits April 28, 2021 20:52
fuzz: Allow to pass min/max to ConsumeTime
fae2c8b
fuzz: Use correct variant of ConsumeRandomLengthString instead of har…
fab646b 
…dcoding a maximum size

This is technically a breaking change.

This allows the fuzz engine to pick the right size,
also larger sizes, if needed.
fuzz: Limit mocktime to MTP in tx_pool targets
fa61ce5 
This is needed for the next commit to generate blocks.

Also, apply the same mocking strategies to both targets.
@DrahtBot DrahtBot added the Tests label Apr 28, 2021
@practicalswift
Copy link
Contributor

practicalswift commented Apr 29, 2021

Concept ACK

Touches only src/test/fuzz/

@maflcko
Copy link
Member Author

maflcko commented Apr 29, 2021

Looks like ci fails due to:

policy/feerate.cpp:26:34: runtime error: signed integer overflow: 1887171891083620 * 5555 cannot be represented in type 'long'
    #0 0x560eb0295c97 in CFeeRate::GetFee(unsigned long) const policy/feerate.cpp:26:34
    #1 0x560eafb6409b in BlockAssembler::addPackageTxs(int&, int&) miner.cpp:378:43
    #2 0x560eafb60c86 in BlockAssembler::CreateNewBlock(CScript const&) miner.cpp:150:5
    #3 0x560eafa21cae in (anonymous namespace)::Finish(FuzzedDataProvider&, (anonymous namespace)::MockedTxPool&, CChainState&) test/fuzz/tx_pool.cpp:89:41
    #4 0x560eafa1e237 in (anonymous namespace)::tx_pool_standard_fuzz_target(Span<unsigned char const>) test/fuzz/tx_pool.cpp:272:5
    
...

SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:26:34 in

@maflcko
Copy link
Member Author

maflcko commented Apr 30, 2021

(ci now green)

@adamjonas
Copy link
Member

adamjonas commented May 5, 2021

Tested fa03232 with DEBUG=1 and observed some nice coverage increases.

My setup is still showing the CI's UB error both in faf3e9d and fa03232, but that just might be due to my own lack of understanding or a compile issue on my part.

@maflcko maflcko force-pushed the 2104-fuzzMempool branch from fa03232 to fa03d0a Compare May 5, 2021 18:12
@maflcko
Copy link
Member Author

maflcko commented May 5, 2021

squashed the commits. bisecting should now work again.

@practicalswift
Copy link
Contributor

practicalswift commented May 5, 2021

Tested ACK fa03d0a

As expected the coverage achieved by the tx_pool fuzzing harness is dramatically improved with this patch applied.

Nice work!

@maflcko maflcko merged commit 1fcf66a into bitcoin:master May 6, 2021
@maflcko maflcko deleted the 2104-fuzzMempool branch May 6, 2021 14:10
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 6, 2021
Merge bitcoin#21798: fuzz: Create a block template in tx_pool targets
3101af3 
fa03d0a fuzz: Create a block template in tx_pool targets (MarcoFalke)
fa61ce5 fuzz: Limit mocktime to MTP in tx_pool targets (MarcoFalke)
fab646b fuzz: Use correct variant of ConsumeRandomLengthString instead of hardcoding a maximum size (MarcoFalke)
fae2c8b fuzz: Allow to pass min/max to ConsumeTime (MarcoFalke)

Pull request description:

  Relatively simple check to ensure a block can always be created from the mempool

ACKs for top commit:
  practicalswift:
    Tested ACK fa03d0a

Tree-SHA512: e613376ccc88591cbe594db14ea21ebc9b2b191f6325b3aa4ee0cd379695352ad3b480e286134ef6ee30f043d486cf9792a1bc7e44445c41045ac8c3b931c7ff
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 1fcf66a into merged_master (Bitcoin PR bitcoin/bitcoin#21798)
b2376cf
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @practicalswift @adamjonas @DrahtBot