fuzz: Rework strong and weak net enum fuzzing #20789

pull MarcoFalke wants to merge 5 commits into bitcoin:master from MarcoFalke:2012-fuzzRefactor changing 7 files +62 −27
  1. MarcoFalke commented at 9:05 pm on December 28, 2020: member

    The fuzz tests have several problems:

    • The array passed to the fuzz engine to pick net_permission_flags is outdated
    • The process_message* targets has the service flags as well as connection type hardcoded, limiting potential coverage
    • The service flags deserialization from the fuzz engine doesn’t allow for easy “exact matches”. The fuzz engine has to explore a 64-bit space to hit an “exact match” (only one bit set)

    Fix all issues in the commits in this pull

  2. MarcoFalke force-pushed on Dec 28, 2020
  3. practicalswift commented at 9:11 pm on December 28, 2020: contributor

    Concept ACK

    Nice improvements!

  4. DrahtBot added the label Tests on Dec 28, 2020
  5. DrahtBot commented at 0:24 am on December 29, 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:

    • #20828 (fuzz: Introduce CallOneOf helper to replace switch-case by MarcoFalke)
    • #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
    • #20228 (addrman: Make addrman a top-level component by jnewbery)

    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.

  6. in src/test/util/net.h:40 in fa9ab9a8c8 outdated 
    35+    NODE_NETWORK,
36+    NODE_BLOOM,
37+    NODE_WITNESS,
38+    NODE_COMPACT_FILTERS,
39+    NODE_NETWORK_LIMITED,
40+};
    vasild commented at 2:25 pm on December 30, 2020:
    I wonder if it is possible to get a warning if a new enum value is added, but this array is forgotten to be updated? I think not :/
    vasild commented at 8:58 am on December 31, 2020:

    It is ok as it is, and I am just wondering if this can be improved wrt minimizing the chance of the actual enum and the “all” arrays going out of sync over time. Maybe one of the below would help:

    o Add a comment next to the enum definition “don’t forget to update ALL_... in src/test/util/net.h if you change this enum”.

    o Define the array immediately below the enum itself.

     0struct Color
 1{
 2    enum E : uint8_t {
 3        RED,
 4        GREEN,
 5        BLUE,
 6    };
 7
 8    static constexpr E all[]{
 9        RED,
10        GREEN,
11        BLUE,
12    };
13};
14
15...
16    std::cout << Color::BLUE << std::endl;
17
18    for (auto& c : Color::all) {
19        std::cout << c << std::endl;
20    }
    practicalswift commented at 9:23 am on December 31, 2020:

    Perhaps we could start using kMaxValue as in the Chromium project. That way we could ​assert that the last element of the ALL_* array is equal to the enum’s kMaxValue.

    Another benefit from using kMaxValue is that we could start using FuzzedDataProvider::ConsumeEnum when fuzzing.

    vasild commented at 12:39 pm on December 31, 2020:

    Yeah, that is another way. enum Network uses this. It only works as long as enum values are left at their defaults:

    https://github.com/bitcoin/bitcoin/blob/f1f26b8d5baec4a45a3a9ba0440cd4eff7af8407/src/netaddress.h#L38

    and also the dummy “max” value has to be explicitly handled in switch without default:

    https://github.com/bitcoin/bitcoin/blob/f1f26b8d5baec4a45a3a9ba0440cd4eff7af8407/src/netaddress.cpp#L145-L146

    MarcoFalke commented at 5:43 pm on December 31, 2020:

    For strong enums, I was thinking about a macro ENUM_CLASS_ALL that writes the enum class and std::array that holds all enum types of that class. Thus, we could keep using PickValueInArray for strong enums. FuzzedDataProvider::ConsumeEnum is an alternative for strong enums that does exactly the same from the perspective of the fuzz engine.

    For weak enums, ConsumeWeakEnum is probably the best bet. I tried kMaxValue and it performed strictly worse in all scenarios, in one it was unable to find a crash at all. And it can’t possibly work for 64-bit enums. Someone should report those bugs to google. See also the benchmark: #20789 (comment)

  7. vasild approved
  8. vasild commented at 2:40 pm on December 30, 2020: member

    ACK fa9ab9a8c

    The chance of 5 random commits to all start with the same two letters is 0.00000002% :eyes: :open_mouth:

  9. MarcoFalke force-pushed on Dec 31, 2020
  10. MarcoFalke commented at 1:11 pm on December 31, 2020: member
    Force pushed to fix a bug. Should be trivial to re-ACK
  11. in src/test/fuzz/process_message.cpp:74 in fadbf6a830 outdated 
    71     p2p_node.nVersion = PROTOCOL_VERSION;
72     p2p_node.SetCommonVersion(PROTOCOL_VERSION);
73     connman.AddTestNode(p2p_node);
74     g_setup->m_node.peerman->InitializeNode(&p2p_node);
75+
76+    // fuzzed_data_provider is fully consumed after this call, don't use it
    vasild commented at 1:32 pm on December 31, 2020:
    What happens if we attempt to consume more bytes after ConsumeRemainingBytes()? Apparently CI was green. Should we abort() if that happens?
    MarcoFalke commented at 1:36 pm on December 31, 2020:
    This happens in normal operation when the fuzz engine starts with an empty byte vector or when existing inputs are trimmed, so it can’t be detected by CI.
  12. vasild approved
  13. vasild commented at 1:33 pm on December 31, 2020: member
    ACK fa
  14. MarcoFalke commented at 1:43 pm on December 31, 2020: member

    Benchmarks

    The goal is to benchmark the complexity of finding a strong and weak match of an enum type. Strong match means the fuzz engine finds an exact value match. (built-in operator== for integers) Weak match means the fuzz engine finds a matching flag. (built-in operator& for integers)

    To reproduce:

    • For all benchmarks clang version 11.0.0 without sanitizers is used.
    • Only the fuzz target process_message_inv is used.
    • -use_value_profile=1 is set at runtime.
    • All patches are on top of this pull request.

    Strong (scoped) enum class

    While C++ doesn’t restrict how scoped enum classes are used, in our code base, scoped enum classes avoid operator&, so this benchmark only considers an exact match.

    The following diff was used for “kMaxValue”:

     0diff --git a/src/net.h b/src/net.h
 1index 6316c73eee..a4930b229c 100644
 2--- a/src/net.h
 3+++ b/src/net.h
 4@@ -178,6 +178,7 @@ enum class ConnectionType {
 5      * AddrMan is empty.
 6      */
 7     ADDR_FETCH,
 8+    kMaxValue=ADDR_FETCH,
 9 };
10 
11 class NetEventsInterface;
12diff --git a/src/net_processing.cpp b/src/net_processing.cpp
13index c5ea2dc85f..964d4e1899 100644
14--- a/src/net_processing.cpp
15+++ b/src/net_processing.cpp
16@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
17                     return;
18                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
19                     AddTxAnnouncement(pfrom, gtxid, current_time);
20+                    assert(!pfrom.IsFeelerConn());
21                 }
22             } else {
23                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
24diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
25index 465452c394..9e345d0c73 100644
26--- a/src/test/fuzz/util.h
27+++ b/src/test/fuzz/util.h
28@@ -307,7 +307,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
29     const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
30     const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider);
31     const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64);
32-    const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES);
33+    const ConnectionType conn_type = fuzzed_data_provider.ConsumeEnum<ConnectionType>();
34     const bool inbound_onion = fuzzed_data_provider.ConsumeBool();
35     if constexpr (ReturnUniquePtr) {
36         return std::make_unique<CNode>(node_id, local_services, my_starting_height, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion);

    The following diff was used for “ArrayEnum”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..964d4e1899 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!pfrom.IsFeelerConn());
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
    kMaxValue (ConsumeEnum) ArrayEnum (PickValueInArray)
    fdp_enum fdp_arr

    No difference should be observed because both calls to the fuzz engine are compiled down to the same ConsumeIntegralInRange. Given the limited experimental data, this seems to hold in practice.

    Weak enum

    Weak match (operator&)

    Diff for “kMaxValue”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..2acd336c1b 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!(pfrom.GetLocalServices() & NODE_NETWORK_LIMITED));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
12diff --git a/src/protocol.h b/src/protocol.h
13index 8af34f58bd..ae863b13d0 100644
14--- a/src/protocol.h
15+++ b/src/protocol.h
16@@ -287,6 +287,7 @@ enum ServiceFlags : uint64_t {
17     // serving the last 288 (2 day) blocks
18     // See BIP159 for details on how this is implemented.
19     NODE_NETWORK_LIMITED = (1 << 10),
20+    kMaxValue=NODE_NETWORK_LIMITED,
21 
22     // Bits 24-31 are reserved for temporary experiments. Just pick a bit that
23     // isn't getting used, or one not being used much, and notify the
24diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
25index 465452c394..7f9a72b585 100644
26--- a/src/test/fuzz/util.h
27+++ b/src/test/fuzz/util.h
28@@ -299,7 +299,7 @@ template <bool ReturnUniquePtr = false>
29 auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = nullopt) noexcept
30 {
31     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegral<NodeId>());
32-    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
33+    const ServiceFlags local_services = fuzzed_data_provider.ConsumeEnum<ServiceFlags>();
34     const int my_starting_height = fuzzed_data_provider.ConsumeIntegral<int>();
35     const SOCKET socket = INVALID_SOCKET;
36     const CAddress address = ConsumeAddress(fuzzed_data_provider);

    Diff for “ArrayEnum”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..2acd336c1b 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!(pfrom.GetLocalServices() & NODE_NETWORK_LIMITED));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
    kMaxValue (ConsumeEnum) ArrayEnum (ConsumeWeakEnum)
    fdp_enum_weak_match our_weak_weak_match

    Exact match (operator==)

    Diff used for “kMaxValue”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..be2a9fac3d 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!(pfrom.GetLocalServices() == NODE_NETWORK_LIMITED));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
12diff --git a/src/protocol.h b/src/protocol.h
13index 8af34f58bd..ae863b13d0 100644
14--- a/src/protocol.h
15+++ b/src/protocol.h
16@@ -287,6 +287,7 @@ enum ServiceFlags : uint64_t {
17     // serving the last 288 (2 day) blocks
18     // See BIP159 for details on how this is implemented.
19     NODE_NETWORK_LIMITED = (1 << 10),
20+    kMaxValue=NODE_NETWORK_LIMITED,
21 
22     // Bits 24-31 are reserved for temporary experiments. Just pick a bit that
23     // isn't getting used, or one not being used much, and notify the
24diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
25index 465452c394..7f9a72b585 100644
26--- a/src/test/fuzz/util.h
27+++ b/src/test/fuzz/util.h
28@@ -299,7 +299,7 @@ template <bool ReturnUniquePtr = false>
29 auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = nullopt) noexcept
30 {
31     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegral<NodeId>());
32-    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
33+    const ServiceFlags local_services = fuzzed_data_provider.ConsumeEnum<ServiceFlags>();
34     const int my_starting_height = fuzzed_data_provider.ConsumeIntegral<int>();
35     const SOCKET socket = INVALID_SOCKET;
36     const CAddress address = ConsumeAddress(fuzzed_data_provider);

    Diff used for “ArrayEnum”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..be2a9fac3d 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!(pfrom.GetLocalServices() == NODE_NETWORK_LIMITED));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
    kMaxValue (ConsumeEnum) ArrayEnum (ConsumeWeakEnum)
    fdp_enum_exact_match our_weak_exact_match

    Double weak match (low bits)

    Diff for “kMaxValue”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..8946a750a8 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!HasAllDesirableServiceFlags(pfrom.GetLocalServices()));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
12diff --git a/src/protocol.h b/src/protocol.h
13index 8af34f58bd..ae863b13d0 100644
14--- a/src/protocol.h
15+++ b/src/protocol.h
16@@ -287,6 +287,7 @@ enum ServiceFlags : uint64_t {
17     // serving the last 288 (2 day) blocks
18     // See BIP159 for details on how this is implemented.
19     NODE_NETWORK_LIMITED = (1 << 10),
20+    kMaxValue=NODE_NETWORK_LIMITED,
21 
22     // Bits 24-31 are reserved for temporary experiments. Just pick a bit that
23     // isn't getting used, or one not being used much, and notify the
24diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
25index 465452c394..7f9a72b585 100644
26--- a/src/test/fuzz/util.h
27+++ b/src/test/fuzz/util.h
28@@ -299,7 +299,7 @@ template <bool ReturnUniquePtr = false>
29 auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = nullopt) noexcept
30 {
31     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegral<NodeId>());
32-    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
33+    const ServiceFlags local_services = fuzzed_data_provider.ConsumeEnum<ServiceFlags>();
34     const int my_starting_height = fuzzed_data_provider.ConsumeIntegral<int>();
35     const SOCKET socket = INVALID_SOCKET;
36     const CAddress address = ConsumeAddress(fuzzed_data_provider);

    Diff for ArrayEnum:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..8946a750a8 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    assert(!HasAllDesirableServiceFlags(pfrom.GetLocalServices()));
 9                 }
10             } else {
11                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
    kMaxValue (ConsumeEnum) ArrayEnum (ConsumeWeakEnum)
    fdp_enum_double our_enum_double

    Double weak match (high bits)

    Diff for “kMaxValue”:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..d0e0167746 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    constexpr auto crash_target = NODE_NETWORK_LIMITED | NODE_WITNESS;
 9+                    assert(!((pfrom.GetLocalServices() & crash_target) == crash_target));
10                 }
11             } else {
12                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
13diff --git a/src/protocol.h b/src/protocol.h
14index 8af34f58bd..ae863b13d0 100644
15--- a/src/protocol.h
16+++ b/src/protocol.h
17@@ -287,6 +287,7 @@ enum ServiceFlags : uint64_t {
18     // serving the last 288 (2 day) blocks
19     // See BIP159 for details on how this is implemented.
20     NODE_NETWORK_LIMITED = (1 << 10),
21+    kMaxValue=NODE_NETWORK_LIMITED,
22 
23     // Bits 24-31 are reserved for temporary experiments. Just pick a bit that
24     // isn't getting used, or one not being used much, and notify the
25diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
26index 465452c394..7f9a72b585 100644
27--- a/src/test/fuzz/util.h
28+++ b/src/test/fuzz/util.h
29@@ -299,7 +299,7 @@ template <bool ReturnUniquePtr = false>
30 auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = nullopt) noexcept
31 {
32     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegral<NodeId>());
33-    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
34+    const ServiceFlags local_services = fuzzed_data_provider.ConsumeEnum<ServiceFlags>();
35     const int my_starting_height = fuzzed_data_provider.ConsumeIntegral<int>();
36     const SOCKET socket = INVALID_SOCKET;
37     const CAddress address = ConsumeAddress(fuzzed_data_provider);

    Diff for ArrayEnum:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index c5ea2dc85f..e8c6b00ad7 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2702,6 +2702,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5                     return;
 6                 } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
 7                     AddTxAnnouncement(pfrom, gtxid, current_time);
 8+                    constexpr auto crash_target = NODE_NETWORK_LIMITED | NODE_WITNESS;
 9+                    assert(!(pfrom.GetLocalServices()&crash_target==crash_target));
10                 }
11             } else {
12                 LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
    kMaxValue (ConsumeEnum) ArrayEnum (ConsumeWeakEnum)
    n/a (aborted without any crash after 5.2*10^6 iterations) our_enum_double_high
  15. practicalswift commented at 4:12 pm on December 31, 2020: contributor

    cr ACK fadbf6a83040b7a8b9f91e27b61adfaf4c5df3fb

    Thanks for providing benchmarks!

  16. MarcoFalke commented at 5:44 pm on December 31, 2020: member
    @vasild btw for the merge script to pick up your ack, you’ll have to provide at least the first 6 chars of the git hash ;)
  17. in src/test/fuzz/util.h:318 in fadbf6a830 outdated 
    319+        return std::make_unique<CNode>(node_id, local_services, my_starting_height, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion);
320+    } else {
321+        return CNode{node_id, local_services, my_starting_height, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion};
322+    }
323 }
324+std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
    mzumsande commented at 7:57 pm on December 31, 2020:
    Trying to build this with clang I get linker errors (multiple definition of ConsumeNodeAsUniquePtr(FuzzedDataProvider&, std::optional<long> const&)). I think that adding an inline to ConsumeNodeAsUniquePtr() would fix this.
    MarcoFalke commented at 11:40 am on January 1, 2021:
    Thanks, fixed. Not sure why I can’t reproduce locally. Are you using the macOS clang?
    mzumsande commented at 1:14 pm on January 1, 2021:
    I am using clang version 6.0.0-1ubuntu2
  18. MarcoFalke force-pushed on Jan 1, 2021
  19. vasild commented at 12:54 pm on January 1, 2021: member

    ACK fac2b6c58ee0bd1efa7a1531af3b882b451c920b

    Both fadbf6a83 and fac2b6c58 compile fine with clang 12.0.0. @MarcoFalke so you were lucky enough to get 15 straight commit ids all starting with fa. I guess you have to have 16777216 times more ~CPU~luck in order to make a commit with arbitrary contents that starts with fac2b6c5 :-D

    Happy New Year! :tada:

  20. mzumsande commented at 0:31 am on January 2, 2021: member

    ACK fac2b6c58ee0bd1efa7a1531af3b882b451c920b

    Code looks good to me, built locally and did some testing, as in the benchmarks.

    As for the worse performance of kMaxValue, I think calling this a “bug” that should be reported is too hard: FuzzedDataProvider::ConsumeEnum() does have a disclaimer “The enum must start at 0 and be contiguous” so it’s just not designed for bitmasks such as enum ServiceFlags. Also, considering that all ConsumeEnum() does is to call ConsumeIntegralInRange between 0 and kMaxValue, it is clear that NODE_NETWORK_LIMITED | NODE_WITNESS, which is > kMaxValue in the benchmark “Double weak match (high bits)” can’t possibly ever hit.

  22. DrahtBot added the label Needs rebase on Jan 2, 2021
  23. MarcoFalke force-pushed on Jan 2, 2021
  24. MarcoFalke commented at 9:12 am on January 2, 2021: member
    Rebased due to trivial conflict in adjacent line
  25. jonatack commented at 9:20 am on January 2, 2021: member
    Concept ACK, first read-through looks very good.
  26. DrahtBot removed the label Needs rebase on Jan 2, 2021
  27. DrahtBot added the label Needs rebase on Jan 2, 2021
  28. fuzz: Use ConsumeNode in process_messages target fa121f058f
  29. fuzz: Use ConsumeNode in process_message target fa42da2d54
  30. fuzz: [refactor] Extract ALL_CONNECTION_TYPES constant faaef9434c
  31. fuzz: Add ConsumeWeakEnum helper, Extract ALL_NET_PERMISSION_FLAGS fa9949b914
  32. fuzz: Use ConsumeWeakEnum for ServiceFlags eeee43bc48
  33. MarcoFalke force-pushed on Jan 2, 2021
  34. MarcoFalke commented at 2:12 pm on January 2, 2021: member
    Rebased after removal of my_starting_height
  35. DrahtBot removed the label Needs rebase on Jan 2, 2021
  36. MarcoFalke commented at 8:11 am on January 7, 2021: member
    Btw, should be (relatively) easy to re-ACK :pray:
  37. mzumsande commented at 3:51 pm on January 7, 2021: member
    ACK eeee43bc48ea7fbacd3c5e3f076f01f04744adb8 after rebase.
  38. MarcoFalke merged this on Jan 7, 2021
  39. MarcoFalke closed this on Jan 7, 2021
  40. MarcoFalke deleted the branch on Jan 7, 2021
  41. sidhujag referenced this in commit fffe61afb7 on Jan 7, 2021
  42. DrahtBot locked this on Aug 16, 2022


MarcoFalke practicalswift DrahtBot vasild mzumsande jonatack

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me