Skip to content

Conversation

ariard
Copy link

@ariard ariard commented Mar 7, 2024

No description provided.

@petertodd
Copy link
Owner

Two issues with this:

  1. The MAX_DATACARRIER_OUTPUTS = 100 limit is much too low for the spirit of libre relay. The default limit should be sufficiently high to be impossible to actually reach; put another way, it should be unlimited.
  2. Every feature we add is a future maintenance burden. In my discussions with miners, what they want most from Libre Relay is good unit tests and a reasonable libre policy that is unlikely to cause them any issues. If they wanted a more limited OP_Return policy, they could just run core.

With that in mind, I'd suggest taking this pull-req and proposing it as a pull-req for Bitcoin Core.

@ariard ariard force-pushed the 2024-libre-relay-datacarriernum branch from 686af03 to a190d25 Compare April 4, 2024 03:36
@ariard
Copy link
Author

ariard commented Apr 4, 2024

Pushed an update, addressing the 2 concerns.

The MAX_DATACARRIER_OUTPUTS = 100 limit is much too low for the spirit of libre relay. The default limit should be > sufficiently high to be impossible to actually reach; put another way, it should be unlimited.

Added a new MAX_DATACARRIER_OUTPUTS_HARDLIMIT=3_999_833 which means one minimal size transaction + 3_999_833 1-byte OP_RETURN. Current transaction issuers can at most make a transaction no bigger than MAX_BLOCK_WEIGHT. MAX_DATACARRIER_OUTPUTS_HARDLIMIT when we see network-level consensus upgrade in matters of block size.

Every feature we add is a future maintenance burden. In my discussions with miners, what they want most from Libre Relay is good unit tests and a reasonable libre policy that is unlikely to cause them any issues. If they wanted a more limited OP_Return policy, they could just run core.

Added a new MAX_DATACARRIER_OUTPUTS_SOFTLIMIT=1 which is matching the max number of OP_RETURN outputs accepted by Bitcoin Core as a transaction-relay policy since release v0.10.0. There is a new check in src/node/mempool_args.cpp verifying that node setting datacarriernum is no strictly superior than MAX_DATACARRIER_OUTPTUS_HARDLIMIT.

Distinction between hard and soft full-node resources limits for denial-of-service mitigation while allowing more flexibility for use-cases experimentations, including accepting transactions yielding higher-income block template is coming from my kernel hacking experience, e.g getrlimit().

@petertodd
Copy link
Owner

Pushed an update, addressing the 2 concerns.

Thing is, you haven't really addressed the second concern. Every bit of code added to Libre Relay is code that needs to be cherry-picked and updated every time Bitcoin Core makes a release; I'm working on that for v27.0rc1 right now. I just don't see a good reason to make the # of OP_Return outputs configurable. What person would want to relay/mine more than 1, but less than ∞? Simply commenting out that check is much simpler, and less work to maintain.

petertodd added a commit that referenced this pull request Apr 9, 2024
Rule #6 prevents fee-rates from being decreased for direct conflicts, and thus
transactions spending confirmed inputs. But it does not prevent fee-rates from
being decreased when unconfirmed inputs are involved as a transaction spending
confirmed inputs can be merged with a transaction spending unconfirmed inputs.
Requiring replacements with unconfirmed inputs to be done one at a time ensures
that fee-rates always increase, while still allowing for RBF of CPFP
transactions.

This is necessary because relace-by-fee-rate assumes that fee-rates can't be
decreased; if they can be you can get infinite cycles of replacements.
@ariard
Copy link
Author

ariard commented Apr 27, 2024

What person would want to relay/mine more than 1, but less than ∞? Simply commenting out that check is much simpler, and less work to maintain.

Any person wishing to operate transaction-relay traffic shaping, where you would like to apply quantitative segments over global transaction-relay traffic (e.g 1/3 third lightning, 1/3 third datacarrier-using colored coins, 1/3 usual transactions). As each segment can have it’s own time-sensitive constraints / liquidity preferences and you wish to optimize income.

As of today, I’m not aware of any mining pools / block template constructors doing so, this is true it sounds very too much flexibility for now. If libre relay focus on maintenance, correct -datacarrierenum might be superfluous for now. Going further, why not removing max OP_RETURN check completely rather than commenting out ? Always easy to re-insert it in mempool’s PreChecks() code path.

argsman.AddArg("-acceptstalefeeestimates", strprintf("Read fee estimates even if they are stale (%sdefault: %u) fee estimates are considered stale if they are %s hours old", "regtest only; ", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES, Ticks<std::chrono::hours>(MAX_FILE_AGE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.AddArg("-datacarriernum", strprintf("Relay and mine transacrtions with max %u data carrier outputs", MAX_DATACARRIER_OUTPUTS_SOFTLIMIT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

Choose a reason for hiding this comment

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

Suggested change
argsman.AddArg("-datacarriernum", strprintf("Relay and mine transacrtions with max %u data carrier outputs", MAX_DATACARRIER_OUTPUTS_SOFTLIMIT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.AddArg("-datacarriernum", strprintf("Relay and mine transactions with max %u data carrier outputs", MAX_DATACARRIER_OUTPUTS_SOFTLIMIT), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

@petertodd
Copy link
Owner

Going further, why not removing max OP_RETURN check completely rather than commenting out ?

IIRC I wanted it to be clear what was there for reference for future rebases. But I don't think I've followed that consistently anyway...

petertodd pushed a commit that referenced this pull request Sep 18, 2024
The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.

For reference, the warning would happen when the separator `-` was not
present.

For example:

  GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json

would result in:

rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
    #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
    #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
    #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
    #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
    #5 0x7f078ebcbbb3  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
    #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
    #7 0x7f078e840a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
    #8 0x7f078e8cdc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
petertodd added a commit that referenced this pull request Sep 18, 2024
Rule #6 prevents fee-rates from being decreased for direct conflicts, and thus
transactions spending confirmed inputs. But it does not prevent fee-rates from
being decreased when unconfirmed inputs are involved as a transaction spending
confirmed inputs can be merged with a transaction spending unconfirmed inputs.
Requiring replacements with unconfirmed inputs to be done one at a time ensures
that fee-rates always increase, while still allowing for RBF of CPFP
transactions.

This is necessary because relace-by-fee-rate assumes that fee-rates can't be
decreased; if they can be you can get infinite cycles of replacements.

Ignoring TRUC test failures for now; will revisit this later.
@petertodd
Copy link
Owner

Closing. No plans to implement this.

@petertodd petertodd closed this Nov 19, 2024
@ariard
Copy link
Author

ariard commented Nov 19, 2024

IIRC I wanted it to be clear what was there for reference for future rebases. But I don't think I've followed that consistently anyway…

Yes, and as you said in the OP as every new feature is a maintenance burden, it's a valuable point not to consider datacarrierenum for now for inclusion in libre-relay. For what it’s worth, an example of use-case for which many data outputs (i.e datacarrierenum) could be worthwhile in the future: ordinals/ord#2405

@petertodd
Copy link
Owner

IIRC I wanted it to be clear what was there for reference for future rebases. But I don't think I've followed that consistently anyway…

Yes, and as you said in the OP as every new feature is a maintenance burden, it's a valuable point not to consider datacarrierenum for now for inclusion in libre-relay. For what it’s worth, an example of use-case for which many data outputs (i.e datacarrierenum) could be worthwhile in the future: ordinals/ord#2405

Libre Relay supports an unlimited number of OP_Return outputs out-of-the-box. The only thing a datacarrierenum option would do is add a limit, which is not the goal of Libre Relay.

petertodd pushed a commit that referenced this pull request Apr 15, 2025
…et_create_transaction

5a26cf7 fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction (brunoerg)

Pull request description:

  This PR limites the value of `m_confirm_target` to avoid `implicit-integer-sign-change`:
  ```
  /ci_container_base/src/wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294967292 (32-bit, unsigned) to type 'int' changed the value to -4 (32-bit, signed)
      #0 0x55b6fd26c021 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/fees.cpp:58:58
      #1 0x55b6fd3ef5ca in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1101:49
      #2 0x55b6fd3ebea5 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1382:16
      #3 0x55b6fccbc154 in wallet::(anonymous namespace)::wallet_create_transaction_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/wallet/test/fuzz/spend.cpp:99:11
      #4 0x55b6fccda45d in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      #5 0x55b6fccda45d in LLVMFuzzerTestOneInput ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./src/test/fuzz/fuzz.cpp:211:5
      #6 0x55b6fc368484 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8a484) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #7 0x55b6fc367b79 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c89b79) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #8 0x55b6fc369796 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8b796) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #9 0x55b6fc369ca7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8bca7) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #10 0x55b6fc35719f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c7919f) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #11 0x55b6fc381826 in main (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca3826) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #12 0x7f934c6661c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
      #13 0x7f934c66628a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
      #14 0x55b6fc34c184 in _start (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c6e184) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)

  SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change /ci_container_base/src/wallet/fees.cpp:58:58
  MS: 0 ; base unit: 0000000000000000000000000000000000000000
  0x2e,0x1,0xb0,0xb8,0x0,0xff,0xff,0xff,0xff,0x60,0x14,0x22,0xff,0xff,0xff,0xff,0xff,0xfd,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb,0x17,0xfc,0xff,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb,
  .\001\260\270\000\377\377\377\377`\024\"\377\377\377\377\377\375\377\377\377\377\377~\371A\010+\027X\013\027\374\377\377\377\377\377\377~\371A\010+\027X\013
  artifact_prefix='./'; Test unit written to ./crash-5627f57ffba7568a500f8379f62c3338978b43f2
  Base64: LgGwuAD/////YBQi///////9//////9++UEIKxdYCxf8////////fvlBCCsXWAs=
  ```

ACKs for top commit:
  maflcko:
    lgtm ACK 5a26cf7
  dergoegge:
    utACK 5a26cf7

Tree-SHA512: a1b129d81d42350cf85ff6fb95cd6982b6aac88467a526ee4b3c9b3313af2f7952c5dfa9886f455756faba399d8356b6c318d7ab2d6318e08fea838bee90b2fe
petertodd pushed a commit that referenced this pull request Sep 13, 2025
Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and:
```bash
export CC=clang
export CXX=clang++
cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address
cmake --build build
export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan"
ctest --test-dir build
```

```bash
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms
********* Finished testing of AddressBookTests *********

=================================================================
==21869==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 88 byte(s) in 1 object(s) allocated from:
    #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
    #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
    #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
    #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18
    #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5
    #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75
    #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13
    #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5
    #8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5
    #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
    #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
    #11 0xffff8cf57c54  (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #12 0xffff8cf5fa18  (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #13 0xffff8cf6067c  (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #14 0xffff8cf610a4  (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    bitcoin#17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30
    bitcoin#18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
    bitcoin#19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
    bitcoin#20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
```

This happens when building using depends:
```bash
Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #1 0xfbff97f8c164  (<unknown module>)
    #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o
    #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)

SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s).
```
petertodd pushed a commit that referenced this pull request Sep 13, 2025
5be31b2 lsan: add more Qt suppressions (fanquake)

Pull request description:

  Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and:
  ```bash
  export CC=clang
  export CXX=clang++
  cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address
  cmake --build build
  export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan"
  ctest --test-dir build
  ```

  ```bash
  Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms
  ********* Finished testing of AddressBookTests *********

  =================================================================
  ==21869==ERROR: LeakSanitizer: detected memory leaks

  Direct leak of 88 byte(s) in 1 object(s) allocated from:
      #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
      #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
      #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
      #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18
      #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5
      #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75
      #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13
      #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5
      #8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5
      #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
      #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
      #11 0xffff8cf57c54  (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #12 0xffff8cf5fa18  (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #13 0xffff8cf6067c  (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #14 0xffff8cf610a4  (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      bitcoin#17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30
      bitcoin#18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
      bitcoin#19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
      bitcoin#20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
  ```

  This happens when building using depends:
  ```bash
  Indirect leak of 24 byte(s) in 1 object(s) allocated from:
      #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #1 0xfbff97f8c164  (<unknown module>)
      #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o
      #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)

  SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s).
  ```

ACKs for top commit:
  maflcko:
    lgtm ACK 5be31b2

Tree-SHA512: 0c33661c7ec83ea9b874c1ee4ee2de513131690287363e216a88560dfb31a59ef563a50af756c86a991583aa64a600a74e20fd5d6a104cf4c0a27532de8d2211
petertodd pushed a commit that referenced this pull request Sep 13, 2025
…xec in RunCommandJSON"

faa1c3e Revert "Merge bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)

Pull request description:

  After a fork() in a multithreaded program, the child can safely
  call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
  until such time as it calls execv.

  The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.

  There was an alternative implementation using `readdir` (bitcoin#32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.

  So temporarily revert this feature.

  A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.

  Fixes bitcoin#32524
  Fixes bitcoin#33015
  Fixes bitcoin#32855

  For reference, a failure can manifest in the GCC debug mode:

  * While `fork`ing, a debug mode mutex is held (by any other thread).
  * The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.

  This may look like the following:

  ```
  (gdb) thread apply all bt

  Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
  #0  0xf7f4f589 in __kernel_vsyscall ()
  #1  0xf79e467e in ?? () from /lib32/libc.so.6
  #2  0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
  #3  0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
  #4  0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
  #5  0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
  #6  0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
  #7  0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
  #8  0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
  #9  0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
  #10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
  #11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
  #12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
  #13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
  #14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
  #15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
  ...
  (truncated, only one thread exists)
  ```

ACKs for top commit:
  fanquake:
    ACK faa1c3e
  darosior:
    ACK faa1c3e

Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a
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.

3 participants