[contrib] message-capture-parser: fix AssertionError on parsing `headers` message #26007

1. 
   theStack commented at 10:43 PM on September 4, 2022: contributor

   If a test framework message's field name is in the list of HASH_INT_VECTORS, we currently assume that it always has to contain a vector of integers and throw otherwise: https://github.com/bitcoin/bitcoin/blob/0ebd4db32b39cb7c505148f090df4b7ac778c307/contrib/message-capture/message-capture-parser.py#L82-L83 (introduced in PR #25367, commit 42bbbba7c83d1e2baad18b4c6f05bad1358eb117).

   However, that assumption is too strict. The (de)serialization field name "headers" is used in two different message types, one for cfcheckpt (where it is serialized as an integer vector), and another time for headers (where it is serialized as a vector of CBlockHeaders). Parsing the latter fails as it is not an integer vector and thus triggers the assert.

   Fix this by adding the integer type check as additional condition to the HASH_INT_VECTORS check rather than asserting. Fixes #25954.

2. message-capture-parser: fix AssertionError on parsing `headers` message

   If a test framework message's field name is in the list of
   `HASH_INT_VECTORS`, we currently assume that it _always_ has to contain
   a vector of integers and throw otherwise (introduced in PR #25367,
   commit 42bbbba7c83d1e2baad18b4c6f05bad1358eb117). However, that
   assumption is too strict. In this concrete case, the (de)serialization
   field name "headers" is used in two different message types, one for
   `cfcheckpt` (where it is serialized as an integer vector), and another
   time for `headers` (where it is serialized as a vector of
   `CBlockHeader`s). Fix by adding the integer type check as additional
   condition to the `HASH_INT_VECTORS` check rather than asserting.
   Fixes #25954.

   644772b9ef
3. MarcoFalke added this to the milestone 24.0 on Sep 5, 2022
4. 
   glozow commented at 3:24 PM on September 8, 2022: member

   ACK 644772b9efffda4dac01aff54042b3162079514d

   I have not taken the time to fully understand the bug, but used the description in #25954 to reproduce the assertion failure on master, and confirmed message-capture-parser.py works with this patch.

   Thanks for reporting, @1440000bytes would you mind taking a look to see if this fixes your issue?

5. MarcoFalke merged this on Sep 9, 2022
6. MarcoFalke closed this on Sep 9, 2022

   ---

7. theStack deleted the branch on Sep 9, 2022
8. 
   ghost commented at 7:43 PM on September 9, 2022: none

   ACK 644772b

   I have not taken the time to fully understand the bug, but used the description in #25954 to reproduce the assertion failure on master, and confirmed message-capture-parser.py works with this patch.

   Thanks for reporting, @1440000bytes would you mind taking a look to see if this fixes your issue?

   It resolved the issue and I could test a few things without wireshark. Thanks @glozow and @theStack

9. sidhujag referenced this in commit e01a908af4 on Sep 11, 2022
10. bitcoin locked this on Sep 9, 2023

Milestone
24.0