zmq: deduplicate 'sequence' publisher message creation/sending #20523

1. 
   theStack commented at 5:56 PM on November 28, 2020: member

   This small PR deduplicates common low-level creation and sending code for the 'sequence' zmq publisher message (methods NotifyBlock{Connect,Disconnect}(), NotifyTransaction{Acceptance,Removal}() in the class CZMQPublishSequenceNotifier) by introducing a helper function.

2. DrahtBot added the label RPC/REST/ZMQ on Nov 28, 2020
3. 
   fanquake commented at 8:54 AM on November 30, 2020: member

   Concept ACK - thanks

4. fanquake requested review from instagibbs on Nov 30, 2020
5. in src/zmq/zmqpublishnotifier.cpp:236 in 5a249c7896 outdated

    227 | @@ -227,50 +228,42 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr
    228 |      return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size());
    229 |  }
    230 |  
    231 | +// Helper function to send a 'sequence' topic message  with the following structure:
    232 | +//    <32-byte hash> | <1-byte label> | <8-byte LE sequence> (optional)
    233 | +static bool SendSequenceMsg(CZMQAbstractPublishNotifier& notifier, uint256 hash, char label, std::optional<uint64_t> sequence = {})
    234 | +{
    235 | +    unsigned char data[32+1+8];
    236 | +    for (unsigned int i = 0; i < 32; i++)
   

   ---

   ---

   

   jonasschnelli commented at 10:13 AM on December 1, 2020:

   even if its move only, I suggest adding brackets to confirm with the code style.

   ---

   

   jonatack commented at 10:49 AM on December 1, 2020:

   s/i++/++i/

   ---

   

   theStack commented at 1:53 PM on December 1, 2020:

   Thanks, done (both suggested changes).

6. in src/zmq/zmqpublishnotifier.cpp:238 in 5a249c7896 outdated

    233 | +static bool SendSequenceMsg(CZMQAbstractPublishNotifier& notifier, uint256 hash, char label, std::optional<uint64_t> sequence = {})
    234 | +{
    235 | +    unsigned char data[32+1+8];
    236 | +    for (unsigned int i = 0; i < 32; i++)
    237 | +        data[31 - i] = hash.begin()[i];
    238 | +    data[32] = label;
   

   ---

   ---

   

   jonasschnelli commented at 10:15 AM on December 1, 2020:

   is there a reason you have dropped the sizeof(uint256) (I think plain constant numbers are hard to read)?

   ---

   

   theStack commented at 1:57 PM on December 1, 2020:

   The reasoning was that if a sizeof is done on a type with a fixed bit-length then one could as well use directly the byte-length. Changed that to use the variable names instead of magic numbers now, see #20523 (comment)

   ---

   

   laanwj commented at 6:00 PM on December 16, 2020:

   Sorry but I tend to agree with @theStack here, the sizes are hardcoded offsets part of the protocol (clients rely on this) and don't depend on the size of the types used in the implemenation (which could change).

7. jonasschnelli added the label Refactoring on Dec 1, 2020
8. 
   jonasschnelli commented at 10:15 AM on December 1, 2020: contributor

   Concept ACK - see my comments above.

9. 
   theStack commented at 1:53 PM on December 1, 2020: member

   Force-pushed with suggested changes:

   • put brackets around loop body to confirm with the code style
   • s/i++/++i/
   • replaced all magic numbers by sizeof(varname) (I don't see much advantage of using sizeof(type) over a number if type also includes a number (e.g. sizeof(uint256) vs. 32); by using sizeof on a variable though, a hypothetical change of the type would not need to change any code in the function body)
10. theStack force-pushed on Dec 1, 2020
11. 
    theStack commented at 6:55 PM on January 17, 2021: member

    Closing this PR, as there hasn't been any activity for more than 6 weeks.

12. theStack closed this on Jan 17, 2021

    ---

13. 
    fanquake commented at 8:39 AM on January 21, 2021: member

    ping @instagibbs

14. fanquake reopened this on Jan 21, 2021

    ---

15. 
    MarcoFalke commented at 10:20 AM on January 21, 2021: member

    review ACK f231ffde04cbd20ba3ec3dc897f6484783ed9e76

16. in src/zmq/zmqpublishnotifier.cpp:235 in f231ffde04 outdated

     227 | @@ -227,50 +228,43 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr
     228 |      return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size());
     229 |  }
     230 |  
     231 | +// Helper function to send a 'sequence' topic message  with the following structure:
     232 | +//    <32-byte hash> | <1-byte label> | <8-byte LE sequence> (optional)
     233 | +static bool SendSequenceMsg(CZMQAbstractPublishNotifier& notifier, uint256 hash, char label, std::optional<uint64_t> sequence = {})
     234 | +{
     235 | +    unsigned char data[sizeof(hash)+sizeof(label)+sizeof(uint64_t)];
    

    ---

    ---

    

    jonatack commented at 12:04 PM on January 21, 2021:

    nit, clang format

        unsigned char data[sizeof(hash) + sizeof(label) + sizeof(uint64_t)];
    

    ---

    

    theStack commented at 1:38 PM on January 21, 2021:

    Done.

17. in src/zmq/zmqpublishnotifier.cpp:241 in f231ffde04 outdated

     236 | +    for (unsigned int i = 0; i < sizeof(hash); ++i) {
     237 | +        data[sizeof(hash) - 1 - i] = hash.begin()[i];
     238 | +    }
     239 | +    data[sizeof(hash)] = label;
     240 | +    if (sequence) WriteLE64(data+sizeof(hash)+sizeof(label), *sequence);
     241 | +    return notifier.SendZmqMessage(MSG_SEQUENCE, data, sequence ? sizeof(data) : sizeof(hash)+sizeof(label));
    

    ---

    ---

    

    jonatack commented at 12:05 PM on January 21, 2021:

    nit, clang format

    -    if (sequence) WriteLE64(data+sizeof(hash)+sizeof(label), *sequence);
    -    return notifier.SendZmqMessage(MSG_SEQUENCE, data, sequence ? sizeof(data) : sizeof(hash)+sizeof(label));
    +    if (sequence) WriteLE64(data + sizeof(hash) + sizeof(label), *sequence);
    +    return notifier.SendZmqMessage(MSG_SEQUENCE, data, sequence ? sizeof(data) : sizeof(hash) + sizeof(label));
    

    ---

    

    theStack commented at 1:38 PM on January 21, 2021:

    Done.

18. in src/zmq/zmqpublishnotifier.cpp:231 in f231ffde04 outdated

     227 | @@ -227,50 +228,43 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr
     228 |      return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size());
     229 |  }
     230 |  
     231 | +// Helper function to send a 'sequence' topic message  with the following structure:
    

    ---

    ---

    

    jonatack commented at 12:05 PM on January 21, 2021:

    nit extra space

    // Helper function to send a 'sequence' topic message with the following structure:
    

    ---

    

    theStack commented at 1:37 PM on January 21, 2021:

    Good eye! :) Done.

19. 
    jonatack commented at 12:08 PM on January 21, 2021: member

    ACK f231ffde04cbd20ba3ec3dc897f6484783ed9e76

20. zmq: deduplicate 'sequence' publisher message creation/sending 962444295d
21. theStack force-pushed on Jan 21, 2021
22. 
    theStack commented at 1:38 PM on January 21, 2021: member

    Thanks for your reviews! Force-pushed with the suggestions of @jonatack.

23. 
    jonatack commented at 7:56 PM on January 21, 2021: member

    Code review re-ACK 962444295d92744e6f671dac2462037f9f3d79d7 per git diff f231ffd 9624442

24. 
    instagibbs commented at 4:53 AM on January 22, 2021: member

    utACK https://github.com/bitcoin/bitcoin/pull/20523/commits/962444295d92744e6f671dac2462037f9f3d79d7

25. fanquake merged this on Jan 22, 2021
26. fanquake closed this on Jan 22, 2021

    ---

27. sidhujag referenced this in commit 44406ab9c3 on Jan 22, 2021
28. DrahtBot locked this on Aug 18, 2022

Contributors

theStackfanquakejonasschnellijonatacklaanwjMarcoFalkeinstagibbs

Review Requested
instagibbs