Skip to content

Fix XDR hash check for ifdef-guarded XDR files#5163

Closed
leighmcculloch wants to merge 1 commit intostellar:masterfrom
leighmcculloch:xdrifdef
Closed

Fix XDR hash check for ifdef-guarded XDR files#5163
leighmcculloch wants to merge 1 commit intostellar:masterfrom
leighmcculloch:xdrifdef

Conversation

@leighmcculloch
Copy link
Member

Fixes CI failures on #5148 by marking ifdef-guarded XDR files with an IFDEF_GUARDED sentinel so the hash check skips those files.

Copilot AI review requested due to automatic review settings March 5, 2026 05:48
@leighmcculloch
Copy link
Member Author

Closing - created by mistake, fix commit not pushed yet

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the build system and core code paths to support conditionally-compiled XDR variants (via --enable-xdr-* flags), addressing CI hash-check issues when XDR files are #ifdef-guarded.

Changes:

  • Add configure-time flags and propagate -DXDR_* defines into XDR header generation.
  • Add #ifdef-guarded handling for TransactionMeta v5 and LedgerEntry ext v2.
  • Introduce an optional HelloWorld operation frame behind XDR_HELLO_WORLD.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/util/MetaUtils.cpp Adds normalization support for TransactionMeta v5 under XDR_TRANSACTION_META_V5.
src/transactions/TransactionMeta.cpp Adds v5 switch cases and selects meta version 5 when enabled.
src/transactions/OperationFrame.cpp Conditionally wires up HELLO_WORLD operation and include.
src/transactions/HelloWorldOpFrame.h Adds optional operation frame declaration guarded by XDR_HELLO_WORLD.
src/transactions/HelloWorldOpFrame.cpp Adds optional operation frame implementation guarded by XDR_HELLO_WORLD.
src/protocol-curr/xdr Updates XDR submodule reference.
src/invariant/LedgerEntryIsValid.cpp Adjusts ledger entry ext-version checks and liquidity pool sponsorship logic under XDR_LEDGER_ENTRY_EXT_V2.
src/Makefile.am Passes $(XDR_DEFINES) to XDR compiler; adds HelloWorld sources to build.
configure.ac Introduces --enable-xdr-* options as automake conditionals.
common.mk Adds XDR_DEFINES/AM_CPPFLAGS plumbing for the new XDR feature flags.

Comment on lines 120 to +121

stellar_core_SOURCES += transactions/HelloWorldOpFrame.h transactions/HelloWorldOpFrame.cpp
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

HelloWorldOpFrame.cpp is entirely #ifdef XDR_HELLO_WORLD-guarded, so when the flag is off this can compile as an empty translation unit and may trigger -Wempty-translation-unit (often promoted to -Werror). Consider adding these sources under an Automake conditional (if XDR_HELLO_WORLD ... endif) so the .cpp is only compiled when the feature is enabled.

Suggested change
stellar_core_SOURCES += transactions/HelloWorldOpFrame.h transactions/HelloWorldOpFrame.cpp
if XDR_HELLO_WORLD
stellar_core_SOURCES += transactions/HelloWorldOpFrame.h transactions/HelloWorldOpFrame.cpp
endif

Copilot uses AI. Check for mistakes.
Comment on lines +139 to 146
#ifdef XDR_LEDGER_ENTRY_EXT_V2
if (le.ext.v() == 1)
#else
if (le.ext.v() != 0)
#endif
{
return "LiquidityPool is sponsored";
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

With XDR_LEDGER_ENTRY_EXT_V2 enabled, this only flags ext version 1 as sponsored, but appears to allow ext version 2 liquidity pool entries to pass without triggering the sponsorship error. If sponsorship is intended to be rejected for all non-zero extension versions, the condition should remain equivalent to le.ext.v() != 0 (or explicitly handle v2 semantics). If v2 is valid-and-not-sponsored, the error message/logic should be updated to reflect what v1/v2 represent.

Copilot uses AI. Check for mistakes.
Comment on lines +554 to +558
#ifdef XDR_TRANSACTION_META_V5
version = 5;
#else
version = 4;
#endif
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

Enabling XDR_TRANSACTION_META_V5 changes the emitted transaction meta version for the same protocol range (V_23+) from 4 to 5. That’s a behavioral/serialization change controlled purely by a build-time flag, which can lead to interoperability issues across nodes/consumers built with different flags. Consider gating version = 5 on an explicit protocol version that defines meta v5 (or a runtime config knob intended for experimental builds), rather than only on a compile-time define.

Suggested change
#ifdef XDR_TRANSACTION_META_V5
version = 5;
#else
version = 4;
#endif
version = 4;

Copilot uses AI. Check for mistakes.
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.

2 participants