Reviewing files that changed from the base of the PR and between db5d000 and 31becc4c2bc153892ca07ab2dbe19fcc1accc822.

• ci/test/00_setup_env_arm.sh (1 hunks)
• ci/test/00_setup_env_native_nowallet.sh (1 hunks)
• ci/test/00_setup_env_native_qt5.sh (1 hunks)
• ci/test/00_setup_env_native_sqlite.sh (1 hunks)
• ci/test/00_setup_env_win64.sh (1 hunks)
• src/Makefile.am (1 hunks)
• src/coinjoin/client.cpp (1 hunks)
• src/coinjoin/util.cpp (0 hunks)
• src/core_io.h (2 hunks)
• src/core_write.cpp (1 hunks)
• src/evo/assetlocktx.cpp (1 hunks)
• src/evo/assetlocktx.h (2 hunks)
• src/evo/cbtx.cpp (0 hunks)
• src/evo/cbtx.h (0 hunks)
• src/evo/chainhelper.cpp (2 hunks)
• src/evo/chainhelper.h (3 hunks)
• src/evo/core_write.cpp (1 hunks)
• src/evo/deterministicmns.cpp (3 hunks)
• src/evo/mnhftx.h (0 hunks)
• src/evo/providertx.h (0 hunks)
• src/governance/object.cpp (2 hunks)
• src/governance/vote.cpp (0 hunks)
• src/llmq/chainlocks.cpp (2 hunks)
• src/llmq/commitment.h (0 hunks)
• src/llmq/dkgsession.cpp (0 hunks)
• src/llmq/instantsend.cpp (4 hunks)
• src/llmq/utils.cpp (0 hunks)
• src/rpc/blockchain.cpp (1 hunks)
• src/rpc/blockchain.h (1 hunks)
• src/rpc/mempool.cpp (1 hunks)
• src/stacktraces.cpp (2 hunks)
• src/test/block_reward_reallocation_tests.cpp (1 hunks)
• src/test/evo_deterministicmns_tests.cpp (1 hunks)
• src/util/irange.h (1 hunks)
• src/util/wpipe.cpp (2 hunks)
• src/validation.cpp (1 hunks)
• src/validation.h (0 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/scriptpubkeyman.h (1 hunks)
• test/lint/lint-circular-dependencies.py (1 hunks)

• src/coinjoin/util.cpp
• src/evo/mnhftx.h
• src/llmq/utils.cpp
• src/governance/vote.cpp
• src/llmq/dkgsession.cpp
• src/evo/cbtx.cpp
• src/llmq/commitment.h
• src/evo/cbtx.h
• src/validation.h
• src/evo/providertx.h

src/stacktraces.cpp (1)

149-165:

Details

✅ Verification successful

Guard Windows-specific ConvertAddress behind ENABLE_STACKTRACES
Wrapping ConvertAddress (and its explanatory comments) in #ifdef ENABLE_STACKTRACES ensures the function is only compiled when stacktraces are enabled, avoiding undefined-symbol errors when ENABLE_STACKTRACES is off.

---

🏁 Script executed:

#!/bin/bash
# Verify there are no stray references to ConvertAddress outside ENABLE_STACKTRACES
rg -n "ConvertAddress" -g '*.cpp' -g '*.h'

Length of output: 396

---

🏁 Script executed:

#!/bin/bash
# Display lines around the ConvertAddress call to check if it's within ENABLE_STACKTRACES
sed -n '200,260p' src/stacktraces.cpp

Length of output: 1591

---

🏁 Script executed:

#!/bin/bash
# Inspect lines around other ConvertAddress references to confirm they are inside ENABLE_STACKTRACES
sed -n '350,500p' src/stacktraces.cpp

Length of output: 3868

---

All ConvertAddress usage is properly guarded
The ConvertAddress function and its sole call are enclosed in #ifdef ENABLE_STACKTRACES. No unguarded references remain.

src/rpc/mempool.cpp (1)

17-17: Added validation.h header

This include addition aligns with the PR's goal of reorganizing header dependencies to resolve circular dependencies related to transaction retrieval. It ensures the RPC mempool code has access to the necessary validation functionality.

src/test/block_reward_reallocation_tests.cpp (1)

14-14: Added node/transaction.h header

This include addition is consistent with similar changes in other test files, properly adjusting dependencies for transaction handling. It reflects the PR's refactoring work to improve modularity by separating transaction functionality from validation.

src/coinjoin/client.cpp (1)

7-7: Replaced validation.h with chain.h

This change replaces a direct dependency on validation.h with chain.h, which is a positive step toward reducing coupling and improving modularity. This aligns with the PR's goal of removing direct inclusion of validation.h across multiple files to reduce circular dependencies.

ci/test/00_setup_env_win64.sh (1)

19-19: Adopt fine‑grained warning suppression
Removing the global NO_WERROR=1 and instead appending targeted -Wno-error flags to CXXFLAGS is a solid improvement. The quoting is correctly balanced and aligns with other CI environments.

ci/test/00_setup_env_native_nowallet.sh (1)

14-14: Consistent targeted warning suppression for no‑wallet build
Removing NO_WERROR=1 and injecting only the specific -Wno-error flags (array-bounds, dangling-reference) keeps the build strict while silencing only known problematic warnings.

ci/test/00_setup_env_arm.sh (1)

28-28: Maintain ABI‑warning suppression in ARM cross‑compile
The consolidated CXXFLAGS correctly disables array-bounds, dangling-reference, and psabi errors—which is necessary for stable ARM builds given GCC’s ABI change warnings.

ci/test/00_setup_env_native_qt5.sh (1)

18-18:

Details

❓ Verification inconclusive

Verify that linker flags are picked up in Qt5 build
Moving LDFLAGS=-static-libstdc++ to the end still looks valid, but flag ordering can sometimes affect configure scripts. Please confirm that this flag is correctly honored.

---

🏁 Script executed:

#!/bin/bash
# Check that -static-libstdc++ appears in the Qt5 configure link flags
grep -R "LDFLAGS" build/config.log | grep static-libstdc++

Length of output: 108

---

I’ll locate where the Qt5 configure log actually lives and inspect the setup script to verify how LDFLAGS is passed.

---

🏁 Script executed:

#!/bin/bash
set -euxo pipefail

# Show the Qt5 environment setup steps to see where config.log should appear
sed -n '1,200p' ci/test/00_setup_env_native_qt5.sh

# Search the repo for any configure logs
find . -type f -name config.log

Length of output: 1114

---

I’ll locate where the Qt5 configure script is actually invoked to confirm that $BITCOIN_CONFIG (and thus LDFLAGS=-static-libstdc++) is passed through.

---

🏁 Script executed:

#!/bin/bash
set -euxo pipefail

# Find all configure invocations in the Qt5 CI scripts
grep -R -n './configure' ci/test

Length of output: 53

---

I’ll locate where BITCOIN_CONFIG is consumed and where configure is invoked in the Qt5 CI flow.

---

🏁 Script executed:

#!/bin/bash
set -euxo pipefail

# List all CI test scripts
find ci/test -maxdepth 1 -type f -name '*.sh' -print

# Find where BITCOIN_CONFIG is referenced
grep -R -n 'BITCOIN_CONFIG' ci/test

# Find actual configure invocations in the CI scripts
grep -R -n '[[:space:]]configure' ci/test

Length of output: 4031

---

Verify that -static-libstdc++ is applied in the Qt5 build

Automatic grepping of build/config.log didn’t locate any logs, so please manually confirm that the linker flag is honored:

• After running the Qt5 configure step, inspect the generated logs or config.status in the build directory (e.g. build/qt*/config.log) to ensure -static-libstdc++ appears in the final link‑flags.
• Alternatively, examine the actual link commands or resulting binaries under build/qt*/lib (for example, run ldd on a Qt5 shared object to verify it does not list libstdc++.so).

If you cannot find the logs in build/, adjust the build invocation or search paths accordingly and reconfirm that LDFLAGS=-static-libstdc++ from BITCOIN_CONFIG is passed through.

src/test/evo_deterministicmns_tests.cpp (1)

13-13: Include <node/transaction.h> to resolve GetTransaction.
The new include ensures the declaration for GetTransaction() is visible in this test file, fixing the missing header issue.

src/Makefile.am (1)

776-776: Add evo/core_write.cpp to build.
The new serialization implementation in evo/core_write.cpp is now correctly compiled into libbitcoin_common.a.

src/evo/assetlocktx.cpp (1)

17-17: Include <node/blockstorage.h> for block lookup.
This header provides BlockManager and LookupBlockIndex, which are used in CheckAssetUnlockTx.

src/rpc/blockchain.cpp (1)

192-192: Switch to free ToJson for CCbTx.
Replaces the removed member ToJson() with the new centralized ::ToJson(*opt_cbTx), leveraging the free function declared in core_io.h.

src/wallet/scriptpubkeyman.cpp (1)

1828-1832: Good addition of the IsMine method for CTxDestination.

This method properly implements the virtual function declared in the base class, providing consistency with the LegacyScriptPubKeyMan implementation. It correctly converts the destination to a script and reuses the existing script-based IsMine method.

src/rpc/blockchain.h (1)

19-19: Good use of forward declaration.

Adding a forward declaration for BlockManager instead of including the full validation.h header reduces compilation dependencies, which can improve build times.

src/util/wpipe.cpp (2)

67-67: Good practice: initializing buffer with zeros.

Explicitly initializing the buffer with {} ensures it starts with all zeros, which is a safer approach than leaving it uninitialized. This helps prevent potential undefined behavior and addresses compiler warnings about uninitialized variables.

---

81-81: Good practice: initializing buffer with zeros.

Same as above - proper initialization of the buffer ensures a known state before writing to the pipe, which is a good defensive programming practice.

src/evo/deterministicmns.cpp (2)

5-5: Include changes align with refactoring approach

The addition of #include <evo/chainhelper.h> and #include <coins.h> reflects the dependency change from using validation.h directly to using the new helper function instead. This reduces circular dependencies.

Also applies to: 17-17

---

55-55: LGTM! Using structured bindings improves code clarity

The change to use GetTransactionBlock with structured bindings is a good refactoring that simplifies the transaction and block hash retrieval pattern. The function properly returns a pair of transaction reference and block hash, and the code correctly uses structured bindings to extract just the transaction reference (ignoring the block hash with _).

src/core_write.cpp (1)

287-287: LGTM! Function calls correctly refactored to use global ToJson functions

These changes are part of a broader refactoring that moves JSON serialization logic from member methods to free-standing functions. Consistently replacing member ToJson() methods with global ::ToJson() functions while dereferencing the optional payload objects is the correct approach.

This refactoring provides several benefits:

1. Separates data structures from serialization logic
2. Centralizes JSON formatting code in one place
3. Reduces dependencies in class implementations
4. Improves maintainability of the serialization code

The scope operator :: ensures the global function is called rather than any member function.

Also applies to: 291-291, 295-295, 299-299, 303-303, 307-307, 311-311, 315-315, 319-319

src/validation.cpp (1)

729-730: Good refactoring to reduce circular dependencies

The code now uses the new GetTransactionBlock() function with structured binding to retrieve the conflicting transaction, replacing what was likely a call to GetTransaction() with more parameters. This implements the PR objective of introducing a proxy function to reduce circular dependencies.

src/governance/object.cpp (2)

10-10: Added include for chainhelper.h to support GetTransactionBlock usage.

This inclusion is necessary to access the new GetTransactionBlock function now being used in this file.

---

471-471: Improved transaction retrieval with structured binding.

The change replaces the previous call to GetTransaction with the new GetTransactionBlock function that returns both the transaction and block hash in a single call, improving code readability by eliminating the need for an output parameter.

-CTransactionRef txCollateral = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, 
-                               m_obj.collateralHash, Params().GetConsensus(), nBlockHash);
+auto [txCollateral, nBlockHash] = GetTransactionBlock(m_obj.collateralHash);

src/llmq/instantsend.cpp (4)

16-16: Added include for chainhelper.h to support GetTransactionBlock usage.

This inclusion is necessary to access the new GetTransactionBlock function used in this file.

---

578-578: Improved transaction retrieval in CheckCanLock with structured binding.

Replaces the previous call to GetTransaction with the new GetTransactionBlock function that returns both the transaction and block hash in a single call. This change improves code readability by eliminating the need for an output parameter.

---

635-635: Improved transaction retrieval in HandleNewInputLockRecoveredSig using structured binding.

Replaces the previous call to GetTransaction with the new GetTransactionBlock function. This continues the pattern of refactoring transaction retrieval throughout the codebase for better readability.

---

1014-1014: Improved transaction retrieval in ProcessInstantSendLock using structured binding.

Replaces the previous call to GetTransaction with the new GetTransactionBlock function, providing a cleaner and more modern API for transaction retrieval.

src/llmq/chainlocks.cpp (2)

13-13: Added include for chainhelper.h to support GetTransactionBlock usage.

This inclusion is necessary to access the new GetTransactionBlock function used in the Cleanup method.

---

639-639: Improved transaction retrieval in Cleanup method with structured binding.

Replaces the previous call to GetTransaction with the new GetTransactionBlock function, which returns both the transaction and block hash in a single call. This improves code readability by eliminating the need for an output parameter.

src/evo/chainhelper.h (3)

21-21: Added forward declaration for CTxMemPool class.

This declaration is necessary because CTxMemPool is used as a parameter type in the new GetTransactionBlock function.

---

33-34: Added CTransactionRef type alias.

Good addition that defines CTransactionRef as a shared pointer to a constant transaction, making the code more readable and consistent where transaction references are used.

---

69-71: Added GetTransactionBlock function declaration with clear documentation.

This well-documented function provides a cleaner API for retrieving transactions and their block hashes, returning them as a pair rather than using an output parameter. The comments with named components (/*tx=*/ and /*hash_block=*/) help clarify what each part of the returned pair represents.

test/lint/lint-circular-dependencies.py (2)

39-40: Updated circular dependency paths for improved code organization

These changes to the expected circular dependencies reflect the refactoring done in this PR, particularly the changes to transaction-related code paths and the introduction of centralized ToJson functions.

---

43-53: Circular dependencies properly updated to match code reorganization

The updated circular dependency paths correctly reflect the new code organization where transaction retrieval logic has been centralized in the new GetTransactionBlock() function in evo/chainhelper.cpp. This helps maintain accurate linting expectations.

src/evo/chainhelper.cpp (2)

9-14: New includes support the transaction retrieval functionality

These additional includes are necessary for the new GetTransactionBlock() function, providing access to transaction indexing and mempool functionality.

---

65-70: Well-designed transaction retrieval function

The new GetTransactionBlock() function effectively consolidates transaction retrieval logic, returning both the transaction and its block hash as a pair. This helps reduce circular dependencies by providing a central location for this functionality.

The early fast-fail path is a good optimization when neither the transaction index nor mempool is available.

src/evo/assetlocktx.h (2)

52-52: Removed ToJson member function in favor of friend declaration

This change is part of the effort to centralize JSON serialization logic and reduce circular dependencies by moving the ToJson implementation to src/evo/core_write.cpp.

---

109-109: Removed ToJson member function in favor of friend declaration

Similar to the change in CAssetLockPayload, this change moves the JSON serialization logic to the new centralized implementation in src/evo/core_write.cpp.

src/evo/core_write.cpp (6)

5-11: Appropriate includes for serialization functions

All necessary includes are present for the various types that need serialization to JSON.

---

14-31: Well-implemented JSON serialization for CAssetLockPayload

This implementation correctly serializes all fields of the CAssetLockPayload class, including proper handling of script public keys and credit outputs.

---

33-43: Correct implementation of CAssetUnlockPayload serialization

All fields are properly serialized, with appropriate type conversions where needed.

---

45-60: CCbTx serialization with proper version checking

The implementation correctly handles different versions of CCbTx by conditionally including fields based on the version number.

---

62-84: CProRegTx serialization with proper type handling

Good implementation that handles conditional serialization based on the node type and properly extracts destination information from script payout.

---

86-144: Remaining ToJson implementations consistently follow pattern

All remaining implementations follow a consistent pattern of creating a UniValue object, populating it with properly formatted fields, and handling conditional serialization based on version or type.

src/core_io.h (2)

14-30: Forward declarations improve code organization

Adding these forward declarations for EVO and LLMQ transaction payload classes supports the architectural improvements mentioned in the PR objectives. This helps resolve circular dependencies by allowing these types to be referenced without including their full declarations.

The organization into a clear section with consistent layout is clean and follows the file's existing style.

---

72-82: Good refactoring of ToJson functions

Moving the ToJson() member methods from their respective class implementations to standalone functions is a good architectural decision that:

1. Resolves the linking errors mentioned in the PR objectives
2. Improves separation of concerns by moving serialization logic out of data classes
3. Ensures these functions are available in libbitcoin_common instead of being scattered across different libraries

The consistent use of [[nodiscard]] is appropriate for these functions that return values. The comment correctly indicates their implementation location in evo/core_write.cpp.

src/wallet/scriptpubkeyman.h (1)

547-547: Good addition of the IsMine override for CTxDestination.

This properly completes the implementation of the DescriptorScriptPubKeyMan class by adding the override for the base class's virtual method, maintaining consistency with the existing IsMine(const CScript&) method. This change aligns with the PR's goal to address compiler warnings.

Reviewing files that changed from the base of the PR and between c449d68b5c08bb29d0306a312110925e8f2ad285 and 6c81a093d034c6c226a96f0bde38b6190486bffa.

• ci/test/00_setup_env_arm.sh (0 hunks)
• ci/test/00_setup_env_native_nowallet.sh (0 hunks)
• ci/test/00_setup_env_native_qt5.sh (0 hunks)
• ci/test/00_setup_env_native_sqlite.sh (0 hunks)
• ci/test/00_setup_env_win64.sh (0 hunks)
• configure.ac (1 hunks)

• ci/test/00_setup_env_arm.sh
• ci/test/00_setup_env_native_qt5.sh
• ci/test/00_setup_env_native_nowallet.sh
• ci/test/00_setup_env_win64.sh
• ci/test/00_setup_env_native_sqlite.sh

• GitHub Check: x86_64-w64-mingw32 / Build depends
• GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
• GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
• GitHub Check: x86_64-apple-darwin / Build depends
• GitHub Check: arm-linux-gnueabihf / Build depends
• GitHub Check: x86_64-pc-linux-gnu / Build depends

configure.ac (2)

469-477: Properly handling -Wattributes warnings based on GCC version

The implementation correctly handles the GCC-specific -Wattributes warnings with additional version-specific filtering to exclude GCC 14 and above.

This is good future-proofing by conditionally applying this handling only for affected GCC versions.

---

479-491: Structured handling of string operation warnings in GCC

The implementation for handling -Wstringop-overread and -Wstringop-overflow warnings follows the same pattern as the other warning flags, maintaining consistency in the codebase.

This completes the set of warning suppressions mentioned in the PR objectives, which allows re-enabling -Werror for GCC builds by handling all the problematic warning cases.

Reviewing files that changed from the base of the PR and between 6c81a093d034c6c226a96f0bde38b6190486bffa and cb7156c.

• ci/test/00_setup_env_arm.sh (0 hunks)
• ci/test/00_setup_env_native_nowallet.sh (0 hunks)
• ci/test/00_setup_env_native_qt5.sh (0 hunks)
• ci/test/00_setup_env_native_sqlite.sh (0 hunks)
• ci/test/00_setup_env_win64.sh (0 hunks)
• configure.ac (1 hunks)
• src/Makefile.am (1 hunks)
• src/coinjoin/client.cpp (1 hunks)
• src/coinjoin/util.cpp (0 hunks)
• src/evo/assetlocktx.cpp (1 hunks)
• src/evo/assetlocktx.h (2 hunks)
• src/evo/cbtx.cpp (0 hunks)
• src/evo/cbtx.h (1 hunks)
• src/evo/chainhelper.cpp (2 hunks)
• src/evo/chainhelper.h (3 hunks)
• src/evo/core_write.cpp (1 hunks)
• src/evo/deterministicmns.cpp (3 hunks)
• src/evo/mnhftx.h (1 hunks)
• src/evo/providertx.h (4 hunks)
• src/governance/object.cpp (2 hunks)
• src/governance/vote.cpp (0 hunks)
• src/llmq/chainlocks.cpp (2 hunks)
• src/llmq/commitment.h (1 hunks)
• src/llmq/dkgsession.cpp (0 hunks)
• src/llmq/instantsend.cpp (4 hunks)
• src/llmq/utils.cpp (0 hunks)
• src/rpc/blockchain.h (1 hunks)
• src/rpc/mempool.cpp (1 hunks)
• src/stacktraces.cpp (2 hunks)
• src/test/block_reward_reallocation_tests.cpp (1 hunks)
• src/test/evo_deterministicmns_tests.cpp (1 hunks)
• src/util/irange.h (1 hunks)
• src/util/wpipe.cpp (2 hunks)
• src/validation.cpp (1 hunks)
• src/validation.h (0 hunks)
• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/scriptpubkeyman.h (1 hunks)
• test/lint/lint-circular-dependencies.py (1 hunks)

• src/governance/vote.cpp
• src/evo/cbtx.cpp
• src/llmq/utils.cpp
• ci/test/00_setup_env_native_nowallet.sh
• ci/test/00_setup_env_native_qt5.sh
• src/llmq/dkgsession.cpp
• ci/test/00_setup_env_win64.sh
• src/coinjoin/util.cpp
• src/validation.h
• ci/test/00_setup_env_native_sqlite.sh
• ci/test/00_setup_env_arm.sh

• src/test/block_reward_reallocation_tests.cpp
• src/evo/assetlocktx.cpp
• src/rpc/mempool.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/coinjoin/client.cpp
• src/evo/providertx.h

• src/evo/mnhftx.h
• src/Makefile.am
• src/util/wpipe.cpp
• src/util/irange.h
• src/stacktraces.cpp
• src/llmq/commitment.h
• src/wallet/scriptpubkeyman.cpp
• src/rpc/blockchain.h
• src/evo/chainhelper.h
• src/evo/assetlocktx.h
• src/evo/chainhelper.cpp
• src/governance/object.cpp
• src/llmq/chainlocks.cpp
• src/evo/cbtx.h
• src/evo/deterministicmns.cpp
• src/wallet/scriptpubkeyman.h
• src/llmq/instantsend.cpp
• configure.ac
• src/validation.cpp

• GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
• GitHub Check: x86_64-apple-darwin / Build depends
• GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
• GitHub Check: arm-linux-gnueabihf / Build depends