backport: bitcoin#15934, #15864, #19188, #18338, #19413, #18571, #18575 (deglobalization part 3)

[kwvg]

Additional Notes

• Depends on merge bitcoin#16240...21526: assumeutxo project backports (part 2) #4703

• During the backporting process, efforts to backport refactor: Remove confusing BlockIndex global bitcoin/bitcoin#19413 resulted in failures in the test suite

   dash@a623f8e4c33f:/dash-src$ ./src/test/test_dash -t wallet_tests
   Running 10 test cases...
   unknown location(0): fatal error: in "wallet_tests/ComputeTimeSmart": memory access violation at address: 0x00000028: no mapping at fault address
   wallet/test/wallet_tests.cpp(334): last checkpoint
  
   *** 1 failure is detected in the test module "Dash Core Test Suite"
   test_dash: ./checkqueue.h:202: CCheckQueue<T>::~CCheckQueue() [with T = CScriptCheck]: Assertion `m_worker_threads.empty()' failed.
   Aborted (core dumped)
  

  • In an attempt to debug this, it was discovered that in setup_common.cpp (source), m_node.peer_logic accepted *m_node.chainman as an argument before its initialisation, resulting in a dereferenced null pointer, that wasn't being used anywhere (yet).
  • The cause was later discovered to be m_node being overridden by every inherited class and that resulted in TestingSetup::TestingSetup's initialisation logic being rendered pointless (which included defining m_node.chainman). This was resolved by backporting test: Avoid overwriting the NodeContext member of the testing setup [-Wshadow-field] bitcoin/bitcoin#19188 (which in turn relies on the backport below)

• Efforts to backport fuzz: Disable debug log file bitcoin/bitcoin#18571 (which plays a part in the backport mentioned above as its responsible for moving the definition of m_node from TestingSetup to its parent, BasicTestingSetup) resulted in failures in the test suite

  dash@a623f8e4c33f:/dash-src$ ./src/test/test_dash  -t wallet_tests
  Running 10 test cases...
  unknown location(0): fatal error: in "wallet_tests/importwallet_rescan": unknown type
  wallet/test/wallet_tests.cpp(203): last checkpoint: "importwallet_rescan" test entry
  
  dash@a623f8e4c33f:/dash-src$ ./src/test/test_dash -t flatfile_tests
  Running 4 test cases...
  test/flatfile_tests.cpp(97): error: in "flatfile_tests/flatfile_allocate": check fs::file_size(seq.FileName(FlatFilePos(0, 0))) == 100 has failed [321 != 100]
  test/flatfile_tests.cpp(101): error: in "flatfile_tests/flatfile_allocate": check fs::file_size(seq.FileName(FlatFilePos(0, 99))) == 100 has failed [321 != 100]
  test/flatfile_tests.cpp(105): error: in "flatfile_tests/flatfile_allocate": check fs::file_size(seq.FileName(FlatFilePos(0, 99))) == 200 has failed [321 != 200]
  test/flatfile_tests.cpp(119): error: in "flatfile_tests/flatfile_flush": check fs::file_size(seq.FileName(FlatFilePos(0, 1))) == 100 has failed [321 != 100]
  
  *** 4 failures are detected in the test module "Dash Core Test Suite"
  

  • Initial suspicion was placed on potential gArgs/m_node.args shenanigans, which resulted in debug and troubleshoot efforts similar to the previous backport. This turned out not to be the case.

  • The next step involved looking for backports done around that time period and through git blame-ing our way through pull requests, Fix wallet unload race condition bitcoin/bitcoin#18338 was backported. This unfortunately, didn't solve the problem either.

  • I decided to focus on the importwallet_rescan failure first and stepped through each line of the test case and the error was triggered when calling dumpwallet. We still didn't know the nature of the error and so I attempted to ask Boost.Test to politely stop catching exceptions and hand over control to the debugger using BOOST_TEST_IGNORE_NON_ZERO_CHILD_CODE (source). That did not help, at all.

    • To add insult to injury, to maximise debug information, I built it with the thread sanitiser enabled and it made stepping through using the debugger much more painful, it was faster to make clean and re-build everything than to step through with TSan enabled.

  • Finally, I decided to set a breakpoint within rpcdump.cpp (where dumpwallet is defined) and stepped through the routine, finally finding the reason why the test failure happened. A JSONRPCError exception (source) which seemed to indicate shenanigans with GetDataDir() was why.

    • git blame-ing through util/system.cpp resulted in backporting Fix datadir handling bitcoin/bitcoin#15864, which didn't help either.

      • Sidenote, this pull request addresses what dash#1494 addresses but as this solution is from upstream and conflicts with our current implementation, we have opted to prefer the upstream variant. This requires a revert.

    • Adding a std::cout print revealed that GetDataDir() was giving us /dash/.dashcore/regtest/wallet.backup instead of something like /tmp/test_common_Dash Core/dc97848d42956a1f35b76515f8b54088a89a8bbc87943435722a16b49197f151/regtest/wallet.backup violating defined behaviour (source), which took me down the gArgs/m_node.args rabbithole again which proved to not help.

    • Reading through the changes made in backport itself, out of curiosity I decided to check every function called by any additions and ParseParameters proved to be the source of our woes.

      Turns out, calling ForceSetArg before ParseParameters means exactly squat as ParseParameters will erase all overrides before parsing its argument (source). Searching through, the source tree of the original bitcoin backport, Merge settings one place instead of five places bitcoin/bitcoin#15934 was discovered. Did it help? Yes. Did it just get backported easily? Nope!

  • Backporting Merge settings one place instead of five places bitcoin/bitcoin#15934 required dealing with some Dash-isms.

    • It required reversal of qt: Fix Repair tab #4715 due to the logic it introduced. Of course, only the implementation of the fix can be reversed, not the fix itself, so while backporting, GetCommandLineArgs needed to be re-worked around the backport.

    • ForceRemoveArg is a Dash-ism that, upon removal, will cause multiple test failures and so removal of it entirely seemed out of the question, thus requiring it to be re-adapted around the backport

      Note, this does result in behaviour change. ForceRemoveArg can no longer remove a network flag, unlike the pre-backport variant. This is due to underlying changes in ArgsManager. Luckily, I saw no such invocations and so continued further.

    • This alone did not solve test failures, which can be attributed to the backport removing the foremost hyphen of the argument key from its internal maps, this was remedied by a scripted change that removed all prefixed hyphens from ForceRemoveArg calls.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[UdjinM6]

me reading PR description 😮

pls see https://github.com/UdjinM6/dash/commits/pr4844

[UdjinM6]

dash_bench is broken... LGTM otherwise

[kwvg]

New changes have been pushed that should fix dash_bench's assertion failure.

[UdjinM6]

LGTM with one nit (see inline comment). ready for review? ;)

[UdjinM6]

18571: should is partial for now I guess. same changes should be applied to src/test/fuzz/process_messages.cpp which we don't have yet (18521 is a part of #4829)

[UdjinM6]

Or just remember to apply this patch to 18521.

[UdjinM6]

utACK 👍

[PastaPastaPasta]

utACK for merging via merge commit