test: fix feature_addrman.py on big-endian systems #27529

1. 
   theStack commented at 6:46 am on April 25, 2023: contributor

   The test feature_addrman.py currently serializes the addrdb without specifying endianness for ints, so the machine’s native byte order is used (see https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment) and the generated peers.dat would be invalid on big-endian systems (our internal (de)serializers always use little-endian, see ser_{read,write}data32). Fix this by explicitly specifying little-endian serialization via the < character in struct.pack(...).

   This is not detected by CI as we unfortunately don’t run functional tests on big-endian systems there (I think we definitely should!).

2. test: fix `feature_addrman.py` on big-endian systems

   The test `feature_addrman.py` currently serializes the addrdb without
   specifying endianness for `int`s, so the machine's native byte order is used (see
   https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment)
   and the generated `peers.dat` would be invalid on big-endian systems.
   Fix this by explicitly specifying little-endian serialization via the
   `<` character in `struct.pack(...)`.

   53c990ad34
3. 
   DrahtBot commented at 6:46 am on April 25, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	MarcoFalke

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on Apr 25, 2023
5. DrahtBot added the label CI failed on Apr 25, 2023
6. 
   maflcko commented at 8:29 am on April 25, 2023: member

   There is ci/test/00_setup_env_s390x.sh, but I guess it runs qemu-user, not qemu-system, so the python part will run on the host endianness. I wonder how hard it would be to spin up qemu-system in the CI env?
7. DrahtBot removed the label CI failed on Apr 28, 2023
8. 
   maflcko commented at 3:18 pm on July 17, 2023: member

   See https://github.com/bitcoin/bitcoin/pull/28087
9. maflcko approved
10. 
    maflcko commented at 5:13 pm on July 25, 2023: member

    review lgtm. Will test later with #28087

    lgtm ACK 53c990ad3406ee945305af84af98d2f020e5f316 🔚

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: lgtm ACK 53c990ad3406ee945305af84af98d2f020e5f316   🔚
    3Q4OT2tQTqBObBcrKRFcksXJ+Hmk4cjXI0J1/jP0WyRJv9KJBww/yqNmqG4wmwyFsKkVg4cftn3+JzTrEqVQqCA==
    

11. maflcko added this to the milestone 26.0 on Jul 25, 2023
12. 
    maflcko commented at 5:41 am on July 26, 2023: member

    tested as well, so my recommendation would be to merge this before #https://github.com/bitcoin/bitcoin/pull/28087
13. fanquake merged this on Jul 26, 2023
14. fanquake closed this on Jul 26, 2023

    ---

15. theStack deleted the branch on Jul 26, 2023
16. fanquake referenced this in commit 492257019d on Aug 9, 2023
17. sidhujag referenced this in commit 5d4c57cc18 on Aug 9, 2023
18. luke-jr referenced this in commit 16cbff2829 on Sep 6, 2023
19. bitcoin locked this on Jul 25, 2024

Contributors
theStack DrahtBot maflcko

Labels
Tests

Milestone
26.0