proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values#242
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. |
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. |
e8bcca3 to
423a789
Compare
|
Added missing |
423a789 to
413f915
Compare
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. |
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:
|
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>
2d3cc77 to
97d8770
Compare
|
Rebased for easier subtree updating in bitcoin/bitcoin#34020. No other changes. |
|
Code review ACK 97d8770. Confirmed no changes since last rebase. @Sjors could you maybe review #246? I want to bump the version after bitcoin/bitcoin#28722 was merged. Then merge newer PR's like this. |
…86d5 789ac8c86d5 test: m_on_cancel called after request finishes 5aa8c36cdd3 test: getParams() called after request cancel 7f954aa5eac race fix: m_on_cancel called after request finishes (bitcoin#34782) 88b4d85099d race fix: getParams() called after request cancel (bitcoin#34777) 4fa90c68015 race fix: worker thread destroyed before it is initialized (bitcoin#34711, bitcoin#34756) 22bec918c97 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error 8a5e3ae6ed2 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values e8d35246918 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9 97d877053b6 proxy-types: add CustomHasField hook for nullable decode paths 8c2f10252c9 refactor: add missing includes to mp/type-data.h b1638aceb40 doc: Bump version 8 > 9 f61af487217 type-map: Work around LLVM 22 "out of bounds index" error git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 789ac8c86d5532438351e06296e7139565ba60d7
22bec918c9 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error 8a5e3ae6ed Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values e8d3524691 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9 97d877053b proxy-types: add CustomHasField hook for nullable decode paths 8c2f10252c refactor: add missing includes to mp/type-data.h b1638aceb4 doc: Bump version 8 > 9 f61af48721 type-map: Work around LLVM 22 "out of bounds index" error git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 22bec918c97d32660c4694c3a8b5af4cdbb88481
…5174 3f28bca5174 test: worker thread destroyed before it is initialized 789ac8c86d5 test: m_on_cancel called after request finishes 5aa8c36cdd3 test: getParams() called after request cancel 7f954aa5eac race fix: m_on_cancel called after request finishes (bitcoin#34782) 88b4d85099d race fix: getParams() called after request cancel (bitcoin#34777) 4fa90c68015 race fix: worker thread destroyed before it is initialized (bitcoin#34711, bitcoin#34756) 22bec918c97 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error 8a5e3ae6ed2 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values e8d35246918 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9 97d877053b6 proxy-types: add CustomHasField hook for nullable decode paths 8c2f10252c9 refactor: add missing includes to mp/type-data.h b1638aceb40 doc: Bump version 8 > 9 f61af487217 type-map: Work around LLVM 22 "out of bounds index" error git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 3f28bca5174f2a231b7fff6772411f8689f3d7e7
…cca9 2fb97e8cca9 race fix: m_on_cancel called after request finishes 846a43aafb4 test: m_on_cancel called after request finishes e69b6bf3f4e race fix: getParams() called after request cancel 75c5425173f test: getParams() called after request cancel f09731e242f race fix: worker thread destroyed before it is initialized 88cacd4239f test: worker thread destroyed before it is initialized 22bec918c97 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error 8a5e3ae6ed2 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values e8d35246918 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9 97d877053b6 proxy-types: add CustomHasField hook for nullable decode paths 8c2f10252c9 refactor: add missing includes to mp/type-data.h b1638aceb40 doc: Bump version 8 > 9 f61af487217 type-map: Work around LLVM 22 "out of bounds index" error git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 2fb97e8cca9feb1df70cf29b2a9895bea2c4c49c
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).