fuzz: Fix assert bug in txorphan target #25624

chinggg · 2022-07-16T02:56:32Z

Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=48914.

It is possible to construct big tx that got rejected in AddTx, so we cannot assume tx will be added successfully. We can only guarantee tx will not be added if orphanage already has it.

chinggg · 2022-07-16T02:58:36Z

include-what-you-use told me to add these lines, is it correct?

❯ iwyu-tool src/test/fuzz/txorphan.cpp -p compile_commands.json

test/fuzz/txorphan.cpp should add these lines:
#include "consensus/validation.h"          // for GetTransactionWeight
#include "node/eviction.h"                 // for NodeId
#include "policy/policy.h"                 // for MAX_STANDARD_TX_WEIGHT
#include "tinyformat.h"                    // for format_error

test/fuzz/txorphan.cpp should remove these lines:
- #include <net.h>  // lines 6-6

The full include-list for test/fuzz/txorphan.cpp:
#include <consensus/amount.h>              // for CAmount
#include <net_processing.h>                // for DEFAULT_MAX_ORPHAN_TRANSAC...
#include <primitives/transaction.h>        // for COutPoint, CTxIn, CTxOut
#include <script/script.h>                 // for CScript
#include <sync.h>                          // for LOCK, WITH_LOCK
#include <test/fuzz/FuzzedDataProvider.h>  // for FuzzedDataProvider
#include <test/fuzz/fuzz.h>                // for FuzzBufferType, LIMITED_WHILE
#include <test/fuzz/util.h>                // for ConsumeTime, CallOneOf
#include <test/util/setup_common.h>        // for MakeNoLogFileContext
#include <txorphanage.h>                   // for TxOrphanage, g_cs_orphans
#include <uint256.h>                       // for uint256
#include <util/check.h>                    // for inline_assertion_check
#include <util/time.h>                     // for SetMockTime
#include <algorithm>                       // for max
#include <cstdint>                         // for uint32_t, uint8_t
#include <memory>                          // for __shared_ptr_access, opera...
#include <set>                             // for set
#include <utility>                         // for swap, pair
#include <vector>                          // for vector
#include "consensus/validation.h"          // for GetTransactionWeight
#include "node/eviction.h"                 // for NodeId
#include "policy/policy.h"                 // for MAX_STANDARD_TX_WEIGHT
#include "tinyformat.h"                    // for format_error

shaavan

Concept ACK

The logic of code changes seems correct, and I agree with the reasoning that:

We can only guarantee tx will not be added if the orphanage already has it.

Though I wonder why this following assumption was initially made?

tx should already be added since it will not be too big in the test

I looked through the file to confirm that the instances that should be added according to the iwyu-tool, and I was able to confirm there addition except for tinyformat.h, as I could not find any use of of format_error in this file.
I have to further check whether the removal of net.h is correct or not.
It would help if you could share the link to the iwyu-tool you are currently using to do the testing.

chinggg · 2022-07-16T13:40:27Z

@shaavan I made the wrong assumption that the tx will not be too big since I expect each tx will only have a few inputs/outputs. Before the PR got merged, I ran the fuzz target locally for 1 day and there was no crash. But OSS-Fuzz can produce a test case that can construct overweight tx with only a few bytes.

For iwyu-tool, I installed it from AUR, which should be the same as the latest upstream. I generated compile_commands.json using bear.

maflcko · 2022-07-16T14:29:21Z

iwyu is run by the CI (https://cirrus-ci.com/task/5279630734131200?logs=ci#L7283):

test/fuzz/txorphan.cpp should add these lines:
#include "consensus/validation.h"          // for GetTransactionWeight
#include "node/eviction.h"                 // for NodeId
#include "policy/policy.h"                 // for MAX_STANDARD_TX_WEIGHT
test/fuzz/txorphan.cpp should remove these lines:
- #include <net.h>  // lines 6-6

Which looks correct.

DrahtBot · 2022-07-17T03:39:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25564 (Move DEFAULT_MAX_ORPHAN_TRANSACTIONS to node/txorphanage.h by MarcoFalke)

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.

maflcko · 2022-07-18T13:05:25Z

lgtm ACK 2315830

2315830 fuzz: Fix assert bug in txorphan target (chinggg) Pull request description: Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=48914. It is possible to construct big tx that got rejected in `AddTx`, so we cannot assume tx will be added successfully. We can only guarantee tx will not be added if orphanage already has it. ACKs for top commit: MarcoFalke: lgtm ACK 2315830 Tree-SHA512: e173bc1a932639746de1192ed238e2e2318899f55371febb598facd0e811d8c54997f074f5e761757e1ffd3ae76d8edf9d673f020b2d97d5762ac656f632be81

d68ca4e Fix `-Wparentheses` gcc warning (Hennadii Stepanov) Pull request description: This PR fixes `-Wparentheses` gcc warning which has been introduced in bitcoin/bitcoin#25624. On the master branch (6d8707b): ``` $ gcc --version gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0 Copyright (C) 2021 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. $ make > /dev/null In file included from ./net.h:29, from ./net_processing.h:9, from test/fuzz/txorphan.cpp:7: test/fuzz/txorphan.cpp: In lambda function: test/fuzz/txorphan.cpp:116:70: warning: suggest parentheses around comparison in operand of ‘==’ [-Wparentheses] 116 | Assert(!have_tx == GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT); | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ ./util/check.h:74:50: note: in definition of macro ‘Assert’ 74 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val) | ^~~ ``` ACKs for top commit: MarcoFalke: ACK d68ca4e Tree-SHA512: 5c98df4d6a6124d048b16eb3caf29bb396223d3394c1f48efc0fe0c8fd334d67dbf64d0b2e40faf9eda6f6a537885abcff05c61e410cfb317737e3dc361791ee

d68ca4e Fix `-Wparentheses` gcc warning (Hennadii Stepanov) Pull request description: This PR fixes `-Wparentheses` gcc warning which has been introduced in bitcoin#25624. On the master branch (6d8707b): ``` $ gcc --version gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0 Copyright (C) 2021 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. $ make > /dev/null In file included from ./net.h:29, from ./net_processing.h:9, from test/fuzz/txorphan.cpp:7: test/fuzz/txorphan.cpp: In lambda function: test/fuzz/txorphan.cpp:116:70: warning: suggest parentheses around comparison in operand of ‘==’ [-Wparentheses] 116 | Assert(!have_tx == GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT); | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ ./util/check.h:74:50: note: in definition of macro ‘Assert’ 74 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val) | ^~~ ``` ACKs for top commit: MarcoFalke: ACK d68ca4e Tree-SHA512: 5c98df4d6a6124d048b16eb3caf29bb396223d3394c1f48efc0fe0c8fd334d67dbf64d0b2e40faf9eda6f6a537885abcff05c61e410cfb317737e3dc361791ee

chinggg force-pushed the fix-fuzz-orphan branch from 9b1c872 to 9df60a9 Compare July 16, 2022 04:35

shaavan reviewed Jul 16, 2022

View reviewed changes

fuzz: Fix assert bug in txorphan target

2315830

chinggg force-pushed the fix-fuzz-orphan branch from 9df60a9 to 2315830 Compare July 17, 2022 00:04

DrahtBot mentioned this pull request Jul 17, 2022

Move DEFAULT_MAX_ORPHAN_TRANSACTIONS to node/txorphanage.h #25564

Closed

maflcko merged commit c395c8d into bitcoin:master Jul 18, 2022

hebasto mentioned this pull request Jul 19, 2022

Fix -Wparentheses gcc warning #25641

Merged

bitcoin locked and limited conversation to collaborators Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fuzz: Fix assert bug in txorphan target #25624

fuzz: Fix assert bug in txorphan target #25624

Uh oh!

chinggg commented Jul 16, 2022 •

edited

Loading

Uh oh!

chinggg commented Jul 16, 2022

Uh oh!

shaavan left a comment

Uh oh!

chinggg commented Jul 16, 2022 •

edited

Loading

Uh oh!

maflcko commented Jul 16, 2022

Uh oh!

DrahtBot commented Jul 17, 2022

Uh oh!

maflcko commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fuzz: Fix assert bug in txorphan target #25624

fuzz: Fix assert bug in txorphan target #25624

Uh oh!

Conversation

chinggg commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinggg commented Jul 16, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

chinggg commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 16, 2022

Uh oh!

DrahtBot commented Jul 17, 2022

Conflicts

Uh oh!

maflcko commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chinggg commented Jul 16, 2022 •

edited

Loading

chinggg commented Jul 16, 2022 •

edited

Loading