Skip to content

test: add map serialization round-trip coverage#297

Merged
ryanofsky merged 2 commits into
bitcoin-core:masterfrom
ViniciusCestarii:test-type-map
Jul 8, 2026
Merged

test: add map serialization round-trip coverage#297
ryanofsky merged 2 commits into
bitcoin-core:masterfrom
ViniciusCestarii:test-type-map

Conversation

@ViniciusCestarii

@ViniciusCestarii ViniciusCestarii commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
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

DrahtBot commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown

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 <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

@ViniciusCestarii

ViniciusCestarii commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Forced-push 645d90b to include map on test.cpp for IWYU

@w0xlt

w0xlt commented Jun 12, 2026

Copy link
Copy Markdown

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Jun 12, 2026
Open
@xyzconstant

xyzconstant commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Looks clean to me, tACK 645d90b

ryanofsky
ryanofsky approved these changes Jul 5, 2026
View reviewed changes

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 645d90b. Looks good but would slightly prefer to keep the generic Pair(T1, T2) type as described below

Comment thread test/mp/test/foo.capnp
message @0 :Text;
}

struct Pair(T1, T2) {

@ryanofsky ryanofsky Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 ViniciusCestarii Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ViniciusCestarii

ViniciusCestarii commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing! forced push 463d073 dropping commit "test: drop mapSize, now covered by map round-trip test" 490585f because mapSize/Pair() test is useful for generic capnp coverage.

ryanofsky
ryanofsky approved these changes Jul 8, 2026
View reviewed changes

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 463d073

Thanks for the update!

@ryanofsky
ryanofsky merged commit 1bd7025 into bitcoin-core:master Jul 8, 2026
13 checks passed
fanquake pushed a commit to bitcoin/bitcoin that referenced this pull request Jul 14, 2026
Squashed 'src/ipc/libmultiprocess/' changes from 28e056576a..e8de5c7b68
6d5f753 
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
463d073cb8 test: rename vBool to vector_bool
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
Sjors added a commit to Sjors/sv2-tp that referenced this pull request Jul 21, 2026
@Sjors
Squashed 'src/ipc/libmultiprocess/' changes from 8412fcdc65..e8de5c7b68
a6b200c 
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections
39a10ce895 proxy: add local connection limit to ListenConnections()
43172f52d9 test: add dedicated ListenConnections coverage
033f812195 doc/version: Bump version 11 > 12
463d073cb8 test: rename vBool to vector_bool
16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs
dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups
400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs
092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper
5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap`
d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs
6450345c98 type: reserve first when reading std::unordered_set
4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers
0e49d91186 Add `makePool` method on `ThreadMap`
5519f7f948 test: recursive async IPC calls
a29ceff40b ci: remove libevent from Core CIs
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
Sjors added a commit to Sjors/sv2-tp that referenced this pull request Jul 21, 2026
@Sjors
Squashed 'src/ipc/libmultiprocess/' changes from 8412fcdc65..e8de5c7b68
79bd483 
e8de5c7b68 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn
9307e68e5a Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13
fac7b9b7f6 refactor: memcpy to std::ranges::copy to work around ubsan warn
1bd7025609 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage
438fdd243d doc: Bump version 12 > 13
28e056576a Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections
39a10ce895 proxy: add local connection limit to ListenConnections()
43172f52d9 test: add dedicated ListenConnections coverage
033f812195 doc/version: Bump version 11 > 12
463d073cb8 test: rename vBool to vector_bool
16bf05dea0 Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs
dd537da9e4 Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups
400291de00 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs
092be515ad Merge bitcoin-core/libmultiprocess#285: Add ReadList helper
5b617880c5 Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap`
d499830415 refactor: rename EventLoop::m_num_clients to m_num_refs
6450345c98 type: reserve first when reading std::unordered_set
4d0f8db5f9 proxy: add ReadList helper and dedup map/set/vector read handlers
0e49d91186 Add `makePool` method on `ThreadMap`
5519f7f948 test: recursive async IPC calls
a29ceff40b ci: remove libevent from Core CIs
85df233845 test: add mapStringInt to foo.capnp to cover map serialization and deserialization

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: e8de5c7b68e0ae21c94ae92aa22e5c3b213f9c12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ViniciusCestarii @DrahtBot @w0xlt @xyzconstant @ryanofsky