tests: Add amount compression/decompression fuzzing to existing fuzzing harness #17917

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:amount-compression-roundtrip changing 2 files +19 −1
  1. practicalswift commented at 3:24 PM on January 13, 2020: contributor

    Small fuzzing improvement:

    Add amount compression/decompression fuzzing to existing fuzzing harness: test compression round-trip (DecompressAmount(CompressAmount(…))).

    Make the domain of CompressAmount(…) explicit.

    Amount compression primer:

        Compact serialization for amounts
    
        Special serializer/deserializer for amount values. It is optimized for
        values which have few non-zero digits in decimal representation. Most
        amounts currently in the txout set take only 1 or 2 bytes to
        represent.
    

    How to test this PR

    $ make distclean
    $ ./autogen.sh
    $ CC=clang CXX=clang++ ./configure --enable-fuzz \
          --with-sanitizers=address,fuzzer,undefined
    $ make
    $ src/test/fuzz/integer
    …
    
  2. DrahtBot added the label Tests on Jan 13, 2020
  3. in src/compressor.h:22 in 77d3b3a16f outdated
      18 | @@ -19,6 +19,7 @@ bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
      19 |  unsigned int GetSpecialScriptSize(unsigned int nSize);
      20 |  bool DecompressScript(CScript& script, unsigned int nSize, const std::vector<unsigned char> &out);
      21 |  
      22 | +// Defined for nAmount <= MAX_MONEY.
    


    MarcoFalke commented at 7:50 PM on January 13, 2020:

    Do you have any evidence to support this claim?


    kiminuo commented at 9:05 PM on January 13, 2020:

    The algorithm works for larger numbers fine. Maybe just reword the comment a bit?

    https://ideone.com/6TkUTp for tinkering


    sipa commented at 9:15 PM on January 13, 2020:

    I believe the algorithm works fine up to approximately 2^64 / 9 (but it's true we only care about it working up to MAX_MONEY).


    practicalswift commented at 9:30 PM on January 13, 2020:

    Yes, it was is the hard upper limit at roughly 2^64 / 9 that I noticed: above that we wrap around (unsigned integer wraparound) :)

    I thought it was worth making our domain assumptions explicit. Explicit is always better than implicit :)


    practicalswift commented at 9:37 PM on January 13, 2020:

    @kiminuo The point of the comment is to make it clear that this function is not intended to be used with input values above MAX_MONEY :)


    kiminuo commented at 7:27 AM on January 14, 2020:

    @practicalswift I know you know. Not sure if others know. :) (Ideone snippet was for me to understand the algorithm better.)

  4. in src/test/fuzz/integer.cpp:67 in d25be0c7a6 outdated
      56 | @@ -56,7 +57,11 @@ void test_one_input(const std::vector<uint8_t>& buffer)
      57 |  
      58 |      const Consensus::Params& consensus_params = Params().GetConsensus();
      59 |      (void)CheckProofOfWork(u256, u32, consensus_params);
      60 | -    (void)CompressAmount(u64);
      61 | +    if (u64 <= MAX_MONEY) {
      62 | +        assert(u64 == DecompressAmount(CompressAmount(u64)));
      63 | +    } else {
      64 | +        (void)CompressAmount(u64);
      65 | +    }
    


    MarcoFalke commented at 9:44 PM on January 13, 2020:

    Could add DecompressAmount(u64);?


    practicalswift commented at 9:49 PM on January 13, 2020:

    That is already done on L65 below :)

  5. MarcoFalke approved
  6. MarcoFalke commented at 9:45 PM on January 13, 2020: member

    ACK on the fuzzer. Not sure about the value of the comment.

  7. practicalswift commented at 10:10 PM on January 13, 2020: contributor

    @MarcoFalke Do you have any suggestion on how to phrase the precondition comment to make it more valuable?

  8. practicalswift commented at 11:16 PM on January 13, 2020: contributor

    @MarcoFalke

    This is an example of a gotcha where the upper limit kicks in, an unsigned integer wraparound occurs and the DecompressAmount(CompressAmount(…)) roundtrip fails:

    int main(void) {
      using CAmount = int64_t;
      CAmount before = -1;
      CAmount after = DecompressAmount(CompressAmount(before));
    
      std::cout << "before=" << before << std::endl;
      std::cout << "after=" << after << std::endl;
      assert(before == after);
    }
    

    -1 (int64_t) gets implicitly converted to std::numeric_limits<uint64_t>::max() which is above the mentioned upper limit around ~2^64 / 9.

    Output:

    comp.cpp:18:32: runtime error: unsigned integer overflow: 16602069666338596453 * 10 cannot be represented in type 'unsigned long'
    comp.cpp:43:7: runtime error: unsigned integer overflow: 2049638230412172401 * 10 cannot be represented in type 'unsigned long'
    before=-1
    after=2049638230412172324
    comp: comp.cpp:59: int main(): Assertion `before == after' failed.
    Aborted (core dumped)
    
  9. practicalswift force-pushed on Jan 14, 2020
  10. in src/compressor.h:22 in 894ef59b35 outdated
      18 | @@ -19,7 +19,13 @@ bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
      19 |  unsigned int GetSpecialScriptSize(unsigned int nSize);
      20 |  bool DecompressScript(CScript& script, unsigned int nSize, const std::vector<unsigned char> &out);
      21 |  
      22 | +// Defined for 0 <= nAmount <= MAX_MONEY.
    


    laanwj commented at 7:19 PM on January 20, 2020:

    practicalswift commented at 11:54 AM on January 21, 2020:

    Good point. Fixed! Thanks!

  11. practicalswift force-pushed on Jan 21, 2020
  12. practicalswift commented at 11:55 AM on January 21, 2020: contributor

    Rebased! :)

  13. tests: Add amount compression/decompression fuzzing to existing fuzzing harness: test compression round-trip 4a7fd7a712
  14. practicalswift force-pushed on Jan 22, 2020
  15. in src/compressor.h:25 in 67068aed6d outdated
      18 | @@ -19,7 +19,17 @@ bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
      19 |  unsigned int GetSpecialScriptSize(unsigned int nSize);
      20 |  bool DecompressScript(CScript& script, unsigned int nSize, const std::vector<unsigned char> &out);
      21 |  
      22 | +/**
      23 | + * Compress amount.
      24 | + *
      25 | + * Please note that nAmount is of type uint64_t and thus cannot be negative.
    


    MarcoFalke commented at 7:19 PM on March 5, 2020:
     * nAmount is of type uint64_t and thus cannot be negative.
    

    "Please note that" is an empty phrase and can be removed.


    practicalswift commented at 8:23 PM on March 5, 2020:

    Good point! Thanks! Now fixed.

  16. MarcoFalke approved
  17. MarcoFalke commented at 7:20 PM on March 5, 2020: member

    ACK. LGTM, just one nit. Let me know if you want to address it or just have this merged as is.

  18. compressor: Make the domain of CompressAmount(...) explicit 7e9c7113af
  19. practicalswift force-pushed on Mar 5, 2020
  20. MarcoFalke merged this on Mar 5, 2020
  21. MarcoFalke closed this on Mar 5, 2020

  22. sidhujag referenced this in commit 8e33e6a5a2 on Mar 7, 2020
  23. sidhujag referenced this in commit 82b27ae26b on Nov 10, 2020
  24. Fabcien referenced this in commit ef7bc4e829 on Jan 19, 2021
  25. practicalswift deleted the branch on Apr 10, 2021
  26. PastaPastaPasta referenced this in commit 3fbdc59b49 on Feb 26, 2022
  27. PastaPastaPasta referenced this in commit b65c380a07 on Feb 26, 2022
  28. PastaPastaPasta referenced this in commit 670b0c48dc on Feb 27, 2022
  29. PastaPastaPasta referenced this in commit 1a7f19997e on Mar 1, 2022
  30. PastaPastaPasta referenced this in commit eb9cbf9e42 on Mar 3, 2022
  31. PastaPastaPasta referenced this in commit 44e7d3869f on Mar 3, 2022
  32. PastaPastaPasta referenced this in commit 97842e95b9 on Mar 5, 2022
  33. PastaPastaPasta referenced this in commit 4b9da8f919 on Mar 7, 2022
  34. DrahtBot locked this on Aug 18, 2022

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: 2026-04-16 15:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me