proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values#242
proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values#242Sjors wants to merge 2 commits intobitcoin-core:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
So far I just took bitcoin/bitcoin@b147783 verbatim, minus the IPC tests which go into Bitcoin Core. Let me know if you need more adjustments. Looks like the LLM found some typos. |
Sjors
force-pushed
the
2026/02/serialize-null
branch
from
fb2622f to
e8bcca3
Compare
|
Whipped up a test inspired by the one in bitcoin/bitcoin@b147783 on the Bitcoin Core side. I fixed the typos and also added a commit to fix two missing includes, that would otherwise need to be added in the test. |
Sjors
force-pushed
the
2026/02/serialize-null
branch
from
e8bcca3 to
423a789
Compare
|
Added missing |
Sjors
force-pushed
the
2026/02/serialize-null
branch
from
423a789 to
413f915
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 413f915. Code here looks good. I think PR title & description need to be updated (also title & description of main commit) because they are referencing CTransactionRef which is not in this repository and also not affected by this change (it requires the CustomHasField(TypeList<CTransaction>...) overload in the original commit).
A good summary of changes here might be "Add CustomHasValue overload to allow mapping non-null Cap'n Proto values to null C++ values" and probably the LLM can turn that into a good title & description not referring to CTransactionRef
Also wondering if you'd want this added to bitcoin/bitcoin#34422 if merged (seems reasonable)
include/mp/proxy-types.h
Outdated
| //! requires function parameters, there's no way to call the function | ||
| //! constructing values for each of the parameters. Similarly there's no way to |
There was a problem hiding this comment.
In commit "ipc: Serialize null CTransactionRef as empty Data" (413f915)
I think there is a missing word and this supposed to say "call the function without constructing values"
That's not necessary. I need this for bitcoin/bitcoin#34020 which can wait for v32. |
Sjors
changed the title
Add a CustomHasField(TypeList<...>, InvokeContext&, const Input&) extension point used by ReadField/CustomReadField to decide whether an input should materialize a C++ value. The default behavior remains input.has(). This enables targeted mappings from specific non-null Cap'n Proto values to null C++ values (for example empty Data in List(Data), where Cap'n Proto C++ cannot currently distinguish null vs empty), without CTransactionRef-specific logic in libmultiprocess. Apply the hook across nullable read paths and add a test covering round-tripping data pointers including null values. Originally proposed here: bitcoin/bitcoin#34020 (comment) Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Sjors
force-pushed
the
2026/02/serialize-null
branch
from
413f915 to
2d3cc77
Compare
|
Tweaked the commit message and add the missing "without". |
|
Code review ACK 2d3cc77, with commit message rewrite and comment fix since last review. FWIW I think new commit message is clear, but new PR description is pretty random and doesn't say what the change does. Not important, but something like this might be better:
|
3 participants
Taken from: bitcoin/bitcoin#34020 (comment)
Let applications override
CustomHasFieldso they can decide to treat certain capnproto values as being unset. For example, when convertingList(Data)tovector<shared_ptr<CTransaction>>, mapping emptyDatafields to null pointers.This safe to do in special cases, like in this example because serialized
CTransactionrepresentations are never empty. It is also useful to do in this case because Cap'n Proto doesn't currently provide any API for distinguishing between unset and empty data values in a list (although they can be distinguished on the wire).