Skip to content

Support checksumming the attachment together with the body#3407

Open
yanglimingcn wants to merge 1 commit into
apache:masterfrom
yanglimingcn:feat/attachment_with_checksum
Open

Support checksumming the attachment together with the body#3407
yanglimingcn wants to merge 1 commit into
apache:masterfrom
yanglimingcn:feat/attachment_with_checksum

Conversation

@yanglimingcn

@yanglimingcn yanglimingcn commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Controller::set_request/response_checksum_type() previously only covered the serialized protobuf body; the attachment (if any) was never protected. Add set_request/response_checksum_attachment(bool) so callers can opt the attachment into the same checksum.

  • baidu_rpc_meta.proto: add RpcMeta.checksum_with_attachment so the receiver knows whether to fold the attachment into verification. Defaults to false, so old peers that don't understand the field keep verifying against the body only (backward compatible).
  • Controller: add the two setters/getters, thread the flag through ClientSettings (Save/ApplyClientSettings) so ParallelChannel/ SelectiveChannel sub-controllers inherit it correctly, and reset it in ResetPods().
  • ChecksumIn: add an optional `attachment' field consumed by checksum handlers.
  • crc32c_checksum.cpp: extend the crc32c over body then attachment (in that fixed order) when requested.
  • baidu_rpc_protocol.cpp: wire checksum_attachment through SerializeRpcMessage/DeserializeRpcMessage and every client/server send/receive path; skip it when progressive attachment reading is enabled since there's no single complete IOBuf to checksum in that case.

Add brpc_checksum_unittest.cpp covering Crc32cCompute/Crc32cVerify directly (including corruption/omission/order sensitivity) and an end-to-end Server/Channel test for both request- and response-side attachment checksums.

What problem does this PR solve?

Issue Number: resolve #3046

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

@yanglimingcn
yanglimingcn force-pushed the feat/attachment_with_checksum branch from 1820541 to 596a80e Compare July 24, 2026 13:04
Controller::set_request/response_checksum_type() previously only
covered the serialized protobuf body; the attachment (if any) was
never protected. Add set_request/response_checksum_attachment(bool)
so callers can opt the attachment into the same checksum.

- baidu_rpc_meta.proto: add RpcMeta.checksum_with_attachment so the
  receiver knows whether to fold the attachment into verification.
  Defaults to false, so old peers that don't understand the field
  keep verifying against the body only (backward compatible).
- Controller: add the two setters/getters, thread the flag through
  ClientSettings (Save/ApplyClientSettings) so ParallelChannel/
  SelectiveChannel sub-controllers inherit it correctly, and reset it
  in ResetPods().
- ChecksumIn: add an optional `attachment' field consumed by checksum
  handlers.
- crc32c_checksum.cpp: extend the crc32c over body then attachment (in
  that fixed order) when requested.
- baidu_rpc_protocol.cpp: wire checksum_attachment through
  SerializeRpcMessage/DeserializeRpcMessage and every client/server
  send/receive path; skip it when progressive attachment reading is
  enabled since there's no single complete IOBuf to checksum in that
  case.

Add brpc_checksum_unittest.cpp covering Crc32cCompute/Crc32cVerify
directly (including corruption/omission/order sensitivity) and an
end-to-end Server/Channel test for both request- and response-side
attachment checksums.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR extends bRPC’s existing request/response checksum support (baidu_std) so the optional attachment can be included in the same checksum as the serialized protobuf body, improving end-to-end data integrity for payloads carried in attachments.

Changes:

  • Adds an opt-in controller flag for including request/response attachments in checksums and threads request-side inheritance through ClientSettings.
  • Extends the checksum input plumbing (ChecksumIn) and updates CRC32C computation to cover body then attachment deterministically.
  • Wires the new flag through baidu_std RPC meta/serialization/deserialization and adds unit + end-to-end tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/brpc_server_unittest.cpp Updates local extern declarations for (de)serialization helpers to accept an optional checksum attachment.
test/brpc_checksum_unittest.cpp Adds focused CRC32C compute/verify tests plus an end-to-end Echo RPC test covering attachment checksumming.
src/brpc/policy/crc32c_checksum.cpp Refactors CRC32C logic to optionally extend over attachment after body using shared helpers.
src/brpc/policy/baidu_rpc_protocol.cpp Threads the attachment checksum option through baidu_std packing/unpacking and checksum verification paths.
src/brpc/policy/baidu_rpc_meta.proto Adds RpcMeta.checksum_with_attachment to signal attachment inclusion to peers.
src/brpc/controller.h Introduces request/response checksum-attachment setters/getters and documents behavior.
src/brpc/controller.cpp Resets new controller fields and includes request-side inheritance via Save/ApplyClientSettings.
src/brpc/checksum.h Extends ChecksumIn to optionally carry an attachment buffer for checksum handlers.
Comments suppressed due to low confidence (1)

src/brpc/policy/baidu_rpc_protocol.cpp:1110

  • PackRpcRequest() always sets meta.checksum_with_attachment when request_checksum_attachment() is true, but SerializeRpcRequest() may skip including the attachment in the checksum when is_response_read_progressively() is set. This produces a request meta that instructs the server to verify body+attachment against a body-only checksum, which will fail for non-empty attachments. Gate the meta flag on the same condition used when computing the checksum.
        meta.set_checksum_type(cntl->request_checksum_type());
        meta.set_checksum_value(accessor.checksum_value());
        if (cntl->request_checksum_attachment()) {
            meta.set_checksum_with_attachment(true);
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 359 to +363
meta.set_checksum_type(cntl->response_checksum_type());
meta.set_checksum_value(accessor.checksum_value());
if (cntl->response_checksum_attachment()) {
meta.set_checksum_with_attachment(true);
}
Comment thread src/brpc/controller.h
Comment on lines +266 to +270
// behavior. This setting is sent to the peer along with the request so
// that it recomputes the checksum over the same range; it is meaningless
// (and rejected, see baidu_rpc_protocol.cpp) together with
// request_will_be_read_progressively() since the attachment is not
// fully buffered before the checksum must be verified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Is CRC validation for attachments under consideration?

2 participants