Enforce Bitcoin mainnet shadow mode by default#110
Conversation
Co-authored-by: dfeen87 <158860247+dfeen87@users.noreply.github.com>
Co-authored-by: dfeen87 <158860247+dfeen87@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR adds a dual-layer safety mechanism to prevent accidental Bitcoin mainnet write operations by enforcing both compile-time and runtime safeguards. The default configuration now operates in "shadow mode" (read-only), requiring explicit opt-in at both build time and runtime to enable transaction broadcasting and signing.
Changes:
- Default
AdapterConfig::readOnlychanged fromfalsetotrue, making all adapters read-only by default - New CMake option
AILEE_BITCOIN_WRITE_ENABLED(default OFF) controls compile-time availability of Bitcoin write operations - Compile-time preprocessor guards added to
buildRawTxandbroadcastRawmethods that throw descriptive errors when write operations are disabled - Tests added to verify the new
readOnlydefault and document compile-time gate state - Build directory
build_write_enabled/added to.gitignorefor write-enabled builds
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/Global_Seven.h | Changed AdapterConfig::readOnly default from false to true with updated comment |
| CMakeLists.txt | Added AILEE_BITCOIN_WRITE_ENABLED option (default OFF) and applied AILEE_BITCOIN_WRITE_DISABLED compile definition to ailee_node and ailee_adapters targets |
| src/l1/BitcoinAdapter.cpp | Added compile-time guards around buildRawTx (wallet signing and UTXO selection) and broadcastRaw (sendrawtransaction) with informative error messages |
| tests/AdapterRegistryTests.cpp | Added test verifying readOnly defaults to true and conditional tests documenting compile-time macro state |
| .gitignore | Added build_write_enabled/ directory for write-enabled build artifacts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| option(AILEE_BITCOIN_WRITE_ENABLED "Enable Bitcoin write operations (sendrawtransaction, sign, etc.)" OFF) | ||
| if(NOT AILEE_BITCOIN_WRITE_ENABLED) | ||
| target_compile_definitions(ailee_node PRIVATE AILEE_BITCOIN_WRITE_DISABLED=1) | ||
| target_compile_definitions(ailee_adapters PRIVATE AILEE_BITCOIN_WRITE_DISABLED=1) | ||
| message(STATUS "Bitcoin write operations: DISABLED (shadow/read-only mode)") | ||
| else() | ||
| message(STATUS "Bitcoin write operations: ENABLED") | ||
| endif() |
There was a problem hiding this comment.
The compile definitions for AILEE_BITCOIN_WRITE_DISABLED are only applied to ailee_node and ailee_adapters targets, but not to the test target ailee_tests (defined at line 437). This means tests will not have the same compile-time configuration as the production code they're testing.
The test at lines 108-119 in tests/AdapterRegistryTests.cpp checks which macro is defined, but it will always execute the "write enabled" path since the test target doesn't receive the AILEE_BITCOIN_WRITE_DISABLED definition.
Consider adding the compile definition to the test target as well to ensure tests run with the same configuration as production code.
| TEST(BitcoinShadowModeTest, WriteDisabledAtCompileTime) { | ||
| // When AILEE_BITCOIN_WRITE_DISABLED is set, the macro is active. | ||
| // This test documents that the compile-time gate is in effect. | ||
| EXPECT_TRUE(true); | ||
| } | ||
| #else | ||
| TEST(BitcoinShadowModeTest, WriteEnabledAtCompileTime) { | ||
| // AILEE_BITCOIN_WRITE_ENABLED=ON was set; write ops are compiled in. | ||
| EXPECT_TRUE(true); |
There was a problem hiding this comment.
These tests only verify that the macro is defined, but don't actually test the behavior of the compile-time guards. The tests always pass with EXPECT_TRUE(true) regardless of whether the guards work correctly.
Consider adding tests that actually verify the runtime behavior:
- For the DISABLED case: test that buildRawTx and broadcastRaw throw the expected runtime_error with the correct message
- For the ENABLED case: test that these functions can execute (or use mocks to verify they reach the RPC layer)
This would provide actual coverage for the safety mechanism rather than just documenting the macro state.
| TEST(BitcoinShadowModeTest, WriteDisabledAtCompileTime) { | |
| // When AILEE_BITCOIN_WRITE_DISABLED is set, the macro is active. | |
| // This test documents that the compile-time gate is in effect. | |
| EXPECT_TRUE(true); | |
| } | |
| #else | |
| TEST(BitcoinShadowModeTest, WriteEnabledAtCompileTime) { | |
| // AILEE_BITCOIN_WRITE_ENABLED=ON was set; write ops are compiled in. | |
| EXPECT_TRUE(true); | |
| TEST(BitcoinShadowModeTest, BitcoinWritesThrowWhenDisabled) { | |
| // When AILEE_BITCOIN_WRITE_DISABLED is set, Bitcoin write operations | |
| // should be blocked at runtime by throwing std::runtime_error. | |
| // buildRawTx should reject any attempt to construct a spendable tx. | |
| EXPECT_THROW( | |
| { | |
| try { | |
| // Parameters are illustrative; exact semantics are handled by the implementation. | |
| auto rawTx = buildRawTx("from_address", "to_address", 1000); | |
| (void)rawTx; | |
| } catch (const std::runtime_error &e) { | |
| // Optionally verify that the error message indicates writes are disabled. | |
| std::string msg = e.what(); | |
| // Do not require a specific string to avoid brittleness, but ensure it's non-empty. | |
| EXPECT_FALSE(msg.empty()); | |
| throw; | |
| } | |
| }, | |
| std::runtime_error); | |
| // broadcastRaw should also be blocked and throw std::runtime_error. | |
| EXPECT_THROW( | |
| { | |
| try { | |
| auto txid = broadcastRaw("deadbeef"); | |
| (void)txid; | |
| } catch (const std::runtime_error &e) { | |
| std::string msg = e.what(); | |
| EXPECT_FALSE(msg.empty()); | |
| throw; | |
| } | |
| }, | |
| std::runtime_error); | |
| } | |
| #else | |
| TEST(BitcoinShadowModeTest, BitcoinWritesSucceedWhenEnabled) { | |
| // When AILEE_BITCOIN_WRITE_DISABLED is not set, Bitcoin write operations | |
| // are compiled in and should be callable without throwing. | |
| std::string rawTx; | |
| EXPECT_NO_THROW( | |
| { | |
| // Construct a raw transaction; implementation defines address/amount handling. | |
| rawTx = buildRawTx("from_address", "to_address", 1000); | |
| }); | |
| // If buildRawTx returns a string, it should be non-empty for a valid call. | |
| EXPECT_FALSE(rawTx.empty()); | |
| std::string txid; | |
| EXPECT_NO_THROW( | |
| { | |
| txid = broadcastRaw(rawTx); | |
| }); | |
| // A successfully broadcast transaction should typically have a non-empty txid. | |
| EXPECT_FALSE(txid.empty()); |
ailee_nodecould sign and broadcast Bitcoin transactions on mainnet without any explicit opt-in. This makes write operations impossible unless deliberately enabled at both runtime and compile time.Changes
include/Global_Seven.h— FlipAdapterConfig::readOnlydefault fromfalse→true. The existing guard inbroadcastTransactionis now always active unless the caller explicitly overridesreadOnly.CMakeLists.txt— Addoption(AILEE_BITCOIN_WRITE_ENABLED ... OFF). When OFF (default), definesAILEE_BITCOIN_WRITE_DISABLED=1on bothailee_nodeandailee_adapters.src/l1/BitcoinAdapter.cpp— Wrap write-capable RPC paths in#if !defined(AILEE_BITCOIN_WRITE_DISABLED)guards:BitcoinTxBuilder::buildRawTx— gateslistunspent,getrawchangeaddress,createrawtransaction,signrawtransactionwithwalletBTCInternal::broadcastRaw— gatessendrawtransactionWhen disabled, each path returns a descriptive error and never touches the RPC layer:
tests/AdapterRegistryTests.cpp— AddAdapterConfigTest::ReadOnlyDefaultIsTrueasserting the new default, andBitcoinShadowModeTestdocumenting the compile-time gate state.To enable write mode explicitly:
cmake -S . -B build -DAILEE_BITCOIN_WRITE_ENABLED=ONNon-Bitcoin adapters are untouched.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.