test: add map serialization round-trip coverage #297

pull ViniciusCestarii wants to merge 2 commits into bitcoin-core:master from ViniciusCestarii:test-type-map changing 3 files +23 −8

ViniciusCestarii commented at 6:35 PM on June 11, 2026: contributor

Add test to ensure map serialization is working and will keep working. Useful for #285.

Also renames v_bool to vector_bool.

Best reviewed commit by commit.
DrahtBot commented at 6:35 PM on June 11, 2026: none



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



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK ryanofsky

Concept ACK w0xlt

Stale ACK xyzconstant

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

No conflicts as of last run.
test: add mapStringInt to foo.capnp to cover map serialization and deserialization 85df233845
ViniciusCestarii force-pushed on Jun 11, 2026
ViniciusCestarii commented at 6:44 PM on June 11, 2026: contributor

Forced-push 645d90b to include map on test.cpp for IWYU
w0xlt commented at 12:41 AM on June 12, 2026: none

Concept ACK
xyzconstant commented at 6:57 PM on June 18, 2026: contributor

Looks clean to me, tACK 645d90b5461fcca540df781859ee8e8c06cb514a
in test/mp/test/foo.capnp:81 in 490585f044
```
  76 | @@ -78,11 +77,6 @@ struct FooMutable {
  77 |      message @0 :Text;
  78 |  }
  79 |  
  80 | -struct Pair(T1, T2) {
```
ryanofsky commented at 7:02 PM on July 5, 2026:

In commit "test: drop mapSize, now covered by map round-trip test" (490585f044355d09dcac054b7f77a4efdffdec48)

I'm not really a fan the commit dropping the mapsize test. It seem to me the generic Pair() type is good to keep because it could be used in other tests testing non-primitive maps, and also it makes helps make sure the code generator and library code works with generics in capnproto, not just non-generic types.

I also don't love renumbering the passDouble method. It seems a little cleaner to me to either fully follow the compatibility rules for updating schema files, or fully not follow them and just update all the field numbers to match field order. It doesn't seem like there is a rationale for numbering fields out of order and also not following compatibility rules.

ViniciusCestarii commented at 10:42 PM on July 5, 2026:

Good points, dropped the commit "test: drop mapSize, now covered by map round-trip test" 490585f. OBS: I still kept the StringIntPair because only pointer types (structs, lists, blobs, and interfaces) can be used as generic parameters%20can%20be%20used%20as%20generic%20parameters).
ryanofsky approved
ryanofsky commented at 7:04 PM on July 5, 2026: collaborator

Code review ACK 645d90b5461fcca540df781859ee8e8c06cb514a. Looks good but would slightly prefer to keep the generic Pair(T1, T2) type as described below
test: rename vBool to vector_bool 463d073cb8
ViniciusCestarii force-pushed on Jul 5, 2026
ViniciusCestarii commented at 10:47 PM on July 5, 2026: contributor

Thanks for reviewing! forced push 463d073cb8df9b7bcab33b3483af7e1759ac67a9 dropping commit "test: drop mapSize, now covered by map round-trip test" https://github.com/bitcoin-core/libmultiprocess/commit/490585f044355d09dcac054b7f77a4efdffdec48 because mapSize/Pair() test is useful for generic capnp coverage.
ryanofsky approved
ryanofsky commented at 5:40 PM on July 8, 2026: collaborator

Code review ACK 463d073cb8df9b7bcab33b3483af7e1759ac67a9

Thanks for the update!
ryanofsky merged this on Jul 8, 2026
ryanofsky closed this on Jul 8, 2026

Type	Reviewers
ACK	ryanofsky
Concept ACK	w0xlt
Stale ACK	xyzconstant

Contributors

Linked (view graph)

#285 Add ReadList helper

test: add map serialization round-trip coverage #297

Reviews

Conflicts