There are a few layers of bullshit to this PR.
First, there is the fact that it adds a functional test gated on a new
config flag which is disabled by default, so it actually adds broken
code with no tests, waiting to ruin your day 520 PRs later when #21935
enables the broken test.
Second, the test appears to be superficially nonsensical because it
generates two transactions from different wallets and tries to compare
them for byte-for-byte equality, which doesn't make sense (at least)
because change outputs are randomly located...so something fishy is
going on.
Of course, in Elements the transactions are *not* equal half the time
because the outputs are permuted, which may have let me quickly figure
out the issue, except...
Third, there is a red herring of a bug where the two transactions have
slightly different feerates. This turns out to be caused by
CWallet::CalculateMaximumSignedTxSize using differently sized dummy
transactions depending on whether watchonly outputs are included (this
fact is conveniently disguised by #17211 slightly changing this logic;
this is an unmerged PR in Core that Elements has a backport of an old
version of). And the two wallets have different watchonly settings.
A sub-red-herring is the fact that this bug results in a discrepancy
of 0.25 vbytes, so it does not appear in Core but does appear in
Elements (there is a 3/16 probability that we should be so unlucky...
we are).
But this is all irrelevant, because...
Fourth, this test is actually super bullshit. The way it works is by
constructing a PSBT legitimately, saving this to disk, then re-"signs"
using the external signer interface by using a mock signer that
COMPLETELY REPLACES THE TRANSACTION UNDER CONSTRUCTION. So it doesn't
matter what the fee output looks like and it doesn't matter what the
order of the outputs. Core does not detect this malfeasance and
neither does Elements. For some reason, Core has a functional test
that explicitly checks that you can do this even though it is insane
and it is hard to think of non-malicious reasons to do it.
Fifth, while Elements fails to detect that its external signer is
actually changing the transaction out from under it, it DOES assume
that this won't happen. In CWallet::SignPSBT it blithely un-replaces
the transaction, which undermines the functional test.
Sixth, the original PR where this test was introduced has comments
locked, so anyone who spent six hours reverse-engineering this idiotic
broken test, and is still feeling charitable enough to discuss it with
the Core developors, can go pound sand.
Anyway, just disabled the broken test and move on with our lives.
aca0e5dcdb Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo)
b3e67f20a0 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
4c3a5dcbfc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
13bd8bb053 Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo)
4d8189f620 scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo)
0f53df47d5 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo)
716de29dd8 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo)
Pull request description:
This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls.
The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: #21789) instead of having it as a global variable (i.e. `gArgs`).
**Notes:**
* First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615274095) but not fully applied in #21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in #21244) This commit can be in a separate PR too.
* Other commits deal with removing of `GetDataDir(net_specific)` function.
* This was originally part of #21244 but it was [left]((https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-633779754)) for a follow up PR.
* I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing.
If you know about a better approach how to do this, please share it here.
ACKs for top commit:
hebasto:
ACK aca0e5dcdb
MarcoFalke:
re-ACK aca0e5dcdb👃
Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
Includes removing some variables from qt/intro.cpp that've been
unused since #13216 and ought to have been removed in the 0.17
rebase, but our linters were not so agressive then. Similarly
fix the BITCOIN_PID_FILE, whose value was incorrectly changed
in the 0.18 rebase.
This option replaces --with-boost-process
This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.
This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
ef712298c3 util: Check for file being NULL in DirectoryCommit (Luke Dashjr)
4574904038 Fix possible data race when committing block files (Evan Klitzke)
220bb16cbe util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke)
ce5cbaea63 util.h: Document FileCommit function (Evan Klitzke)
844d650eea util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke)
f6cec0bcaf util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke)
Pull request description:
Reviving #12696
ACKs for top commit:
laanwj:
Code review ACK ef712298c3
Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce
This commit adds signet support, which is a little bit silly/redundant for us :)
Was a surprisingly easy merge to handle, and hopefully in future Core is more
mindful of signed blocks when they are changing code architecture.
I had to change a couple lines of src/signet.cpp to add blank assets to the
CTxOuts and to find transaction input scriptWitnesses. No need to add any
other tx witness data (and the CAsset()s that I did add to make things compile
won't be used..) because signet will always have g_con_elementsmode off.
