Age | Commit message (Collapse) | Author |
|
|
|
|
|
|
|
|
|
Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.
Note:
This new struct, down the commits line, will contain other `AvailableCoins` useful results.
|
|
p2sh-segwit in createmultisig
3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0190c634b5f1c85f749437f4209cc36 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes #25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
https://github.com/bitcoin/bitcoin/blob/192d639a6b1bd0feaa52e6ea4e63e33982704c32/src/rpc/output_script.cpp#L166-L169
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
laanwj:
Code review ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
|
|
createmultisig and addmultisigaddress
|
|
|
|
6fbb0edac22c63f1b723f731c2601b1d46879a58 Set effective_value when initializing a COutput (ishaanam)
Pull request description:
Previously in COutput, effective_value was initialized as the absolute value of the txout and the fee as 0. effective_value along with the fee was calculated outside of the COutput constructor and set after the object had been initialized.
These changes will allow either the fee or the feerate to be passed in a COutput constructor and the fee and effective_value are calculated and set in the constructor. As a result, AvailableCoins also needs to be passed the feerate when utxos are being spent. When balance is calculated or the coins are being listed and feerate is neither available nor required, AvailableCoinsListUnspent is used instead, which runs AvailableCoins while providing the default value for `feerate`. Unit tests for the calculation of effective value have also been added.
ACKs for top commit:
achow101:
re-ACK 6fbb0edac22c63f1b723f731c2601b1d46879a58
Xekyo:
re-ACK 6fbb0edac22c63f1b723f731c2601b1d46879a58
w0xlt:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/25083/commits/6fbb0edac22c63f1b723f731c2601b1d46879a58
furszy:
Looks good, have been touching this area lately, code review ACK 6fbb0eda.
Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
|
|
addresses were found in the address book
baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658ad93f7b628eb2a3411fec2265d73fb rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)
Pull request description:
Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.
If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).
ACKs for top commit:
achow101:
ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
theStack:
re-ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/25122/commits/baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
|
|
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
|
|
destinations were found for the input label.
If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
|
|
`-deprecatedrpc=exclude_coinbase` logic
a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836c4339aa7f14fc1c9583d86dd5c388a rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner)
Pull request description:
Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
|
|
fa9af218780b7960d756db80c57222e5bf2137b1 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af218780b7960d756db80c57222e5bf2137b1
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
|
|
|
|
ac6fbf2c83578129a0397d0d0dc9b1c6bdb30701 tidy: use modernize-use-default-member-init (fanquake)
7aa40f55636be565441a9e0af8de0a346bfa4da2 refactor: use C++11 default initializers (fanquake)
Pull request description:
Refactor and then enable [`modernize-use-default-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html) in our `clang-tidy` job.
Top commit has no ACKs.
Tree-SHA512: 536b406f20639f8c588fe9e96175ec60c7bb825506b2670b562370b2f572801c24203c483443be3c199e1b958c0765d4532e57c57a4e78689162a1dd422d844f
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's|\<get_int64\>|getInt<int64_t>|g' $(git grep -l get_int ':(exclude)src/univalue')
sed -i 's|\<get_int\>|getInt<int>|g' $(git grep -l get_int ':(exclude)src/univalue')
-END VERIFY SCRIPT-
|
|
from non-test/benchmarking code
a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0 Add more proper thread safety annotations (Hennadii Stepanov)
8cfe93e3fcf263bf059f738d5e7d9c94901a7c5a Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov)
ca446f2c59720c1575aeeab9c9d636d98ce8528c Add proper thread safety annotation to `CachedTxGetAvailableCredit()` (Hennadii Stepanov)
Pull request description:
In non-test/benchmarking code, there are three cases of the `NO_THREAD_SAFETY_ANALYSIS` annotation which are accompanied with `TODO` comments.
This PR adds proper thread safety annotations instead of `NO_THREAD_SAFETY_ANALYSIS`.
ACKs for top commit:
laanwj:
Code review ACK a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0
Tree-SHA512: 806d72eebc1edf088bfa435c8cd11465be0de6789798dd92abd008425516768acb864a73d834a49d412bb10f7fccfb47473f998cb72739dab6caeef6bcfaf191
|
|
|
|
|
|
CreateTransaction() as optional struct
4c5ceb040cf50d24201903a9200fb23be88d96fb wallet: CreateTransaction(): return out-params as (optional) struct (Sebastian Falbesoner)
c9fdaa5e3ae09b45be6a5c2d4ee6b1e8cef9d8a8 wallet: CreateTransactionInternal(): return out-params as (optional) struct (Sebastian Falbesoner)
Pull request description:
The method `CWallet::CreateTransaction` currently returns several values in the form of out-parameters:
* the actual newly created transaction (`CTransactionRef& tx`)
* its required fee (`CAmount& nFeeRate`)
* the position of the change output (`int& nChangePosInOut`) -- as the name suggests, this is both an in- and out-param
By returning these values in an optional structure (which returns no value a.k.a. `std::nullopt` if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don't have to create dummy variables for results that they are not interested in.
Note that the names of the replaced out-variables were kept in `CreateTransactionInternal` to keep the diff minimal. Also, the fee calculation data (`FeeCalculation& fee_calc_out`) would be another candidate to put into the structure, but `FeeCalculation` is currently an opaque data type in the wallet interface and I think it should stay that way.
As a potential follow-up, I think it would make sense to also do the same refactoring for `CWallet::FundTransaction`, which has a very similar parameter structure.
Suggested by laanwj in https://github.com/bitcoin/bitcoin/pull/20588#issuecomment-739838428.
ACKs for top commit:
achow101:
re-ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
Xekyo:
ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
w0xlt:
crACK https://github.com/bitcoin/bitcoin/pull/20640/commits/4c5ceb040cf50d24201903a9200fb23be88d96fb
Tree-SHA512: 27e5348bbf4f698713002d40c834dcda59c711c93207113e14522fc6d9ae7f4d8edf1ef6d214c5dd62bb52943d342878960ca333728828bf39b645a27d55d524
|
|
|
|
performance
f336ff7f213564909cf5f9742618cc6ec87600fd rpc: avoid expensive `IsMine` calls in `GetReceived` tally (Sebastian Falbesoner)
a7b65af2a4450663d4a20a617c59dac87b34fb36 rpc: avoid scriptPubKey<->CTxDestination conversions in `GetReceived` tally (Sebastian Falbesoner)
Pull request description:
The RPC calls `getreceivedbyaddress`/`getreceivedbylabel` both use the internal helper function `GetReceived` which was introduced in PR #17579 to deduplicate tallying code. For every wallet-related transaction output, the following unnecessary operations are currently performed in the tally loop, leading to a quite bad performance (as reported in #23645):
- converting from CScript -> TxDestination (`ExtractDestination(...)`), converting from TxDestination -> CScript (`CWallet::IsMine(const CTxDestination& dest)`); this can be avoided by directly using output scripts in the search set instead of addresses (first commit)
- checking if the iterated output script belongs to the wallet by calling `IsMine`; this can be avoided by only adding addresses to the search set which fulfil `IsMine` in the first place (second commit)
### Benchmark results
The functional test [wallet_pr23662.py](https://github.com/theStack/bitcoin/blob/pr23662_benchmarks/test/functional/wallet_pr23662.py) (not part of this PR) creates transactions with 15000 different addresses:
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine set (received)
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine not set (sent)
- 5000 outputs (500 txs with 10 outputs each) without label set, IsMine not set (sent)
Then, the time is measured for calling `getreceivedbyaddress` and `getreceivedbylabel`, the latter with two variants. Results on my machine:
| branch | `getreceivedbyaddress` (single) | `getreceivedbylabel` (single) | `getreceivedbylabel` (10000) |
|--------------------|---------------------------------|-------------------------------|------------------------------|
| master | 406.13ms | 425.33ms | 446.58ms |
| PR (first commit) | 367.18ms | 365.81ms | 426.33ms |
| PR (second commit) | 3.96ms | 4.83ms | 339.69ms |
Fixes #23645.
ACKs for top commit:
achow101:
ACK f336ff7f213564909cf5f9742618cc6ec87600fd
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/23662/commits/f336ff7f213564909cf5f9742618cc6ec87600fd
furszy:
Code ACK f336ff7f
Tree-SHA512: 9cbf402b9e269713bd3feda9e31719d9dca8a0dfd526de12fd3d561711589195d0c50143432c65dae279c4eab90a4fc3f99e29fbc0452fefe05113e92d129b8f
|
|
|
|
4637bbe448ae7370528f40092ce230c32602a6d6 rpc: Explain active and internal in listdescriptors (Andrew Chow)
Pull request description:
The current help text for active and internal in listdescriptors is not particularly helpful. They require the reader to already know what those terms mean. This help text is updated to actually explain the definitions of those words in context of a descriptor wallet.
ACKs for top commit:
S3RK:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
jarolrod:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/24977/commits/4637bbe448ae7370528f40092ce230c32602a6d6
Tree-SHA512: 0af2c04f3b9920799cf616ad618bde9248eb9f74cc28f443b5b0f6646deba76e9b1415aca0865ad3bcc24aa6af0e9d07ad7b7cd80f0fe80838cf847f1b944426
|
|
fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2 Remove not needed clang-format off comments (MarcoFalke)
Pull request description:
It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments.
Can be reviewed with `--word-diff-regex=. --ignore-all-space`
Looks like this was initially added in commit d9d79576f423cd9c5cef4547c7e3648dbb339460 to accommodate a linter that has since been removed and replaced by a functional test.
ACKs for top commit:
laanwj:
Code review ACK fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2
fanquake:
ACK fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2
Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
|
|
helper (boost::split replacement)
a62e84438d27ee6213219fe2c233e58814fcbb5d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d94a0fdee4ff917e81360d7ae6bd8110 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e412056ed22d364538f0da3d5d71946d refactor: introduce single-separator split helper `SplitString` (Sebastian Falbesoner)
Pull request description:
This PR adds a simple string split helper `SplitString` that takes use of the spanparsing `Split` function that was first introduced in #13697 (commit fe8a7dcd78cfeedc9a7c705e91384f793822912b). This enables to replace most calls to `boost::split`, in the cases where only a single separator character is used. Note that while previous attempts to replace `boost::split` were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all `boost::split` instances can be tackled.
As a possible optimization, one could return a vector of `std::string_view`s (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.
ACKs for top commit:
martinus:
Code review ACK a62e84438d27ee6213219fe2c233e58814fcbb5d. Ran all tests. I also like that with `boost::split` it was not obvious that the resulting container was cleared, and with `SplitString` API that's obvious.
Tree-SHA512: 10cb22619ebe46831b1f8e83584a89381a036b54c88701484ac00743e2a62cfe52c9f3ecdbb2d0815e536c99034558277cc263600ec3f3588b291c07eef8ed24
|
|
|
|
|
|
The current help text for active and internal in listdescriptors is not
particularly helpful. They require the reader to already know what those
terms mean. This help text is updated to actually explain the
definitions of those words in context of a descriptor wallet.
|
|
Can be reviewed with --word-diff-regex=. --ignore-all-space
|
|
function and NONFATAL_UNREACHABLE macro
ee02c8bd9aedad8cfd3c2618035fe275da025fb9 util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9
MarcoFalke:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
|
|
UNREACHABLE macros
|
|
Without this, trying to disable a wallet flag results in an Internal bug
detected.
|
|
This helper uses spanparsing::Split internally and enables to replace
all calls to boost::split where only a single separator is passed.
Co-authored-by: Martin Ankerl <Martin.Ankerl@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
|
|
|
|
|
|
bb84b7145b31dbfdcb4cf0b9b6e612a57e573993 add tests for no recipient and using send_max while inputs are specified (ishaanam)
49090ec4025152c847be8a5ab6aa6f379e345260 Add sendall RPC née sweep (Murch)
902793c7772e5bdd5aae5b0d20a32c02a1a6dc7c Extract FinishTransaction from send() (Murch)
6d2208a3f6849a3732af6ff010eeea629b9b10d0 Extract interpretation of fee estimation arguments (Murch)
a31d75e5fb5c1304445d698595079e29f3cd3a3a Elaborate error messages for outdated options (Murch)
35ed094e4b0e0554e609709f6ca1f7d17096882c Extract prevention of outdated option names (Murch)
Pull request description:
Add sendall RPC née sweep
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _given UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _given budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending a given set of
UTXOs such as paying the value from one or more specific UTXOs, emptying
a wallet, or burning dust, we realized that there are some cases in
which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual
computation of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific
subset of the wallet's UTXO pool (by default all of them) and assigns
the funds to one or more receivers. Receivers can either be specified
with a given amount or receive an equal share of the remaining
unassigned funds. At least one recipient must be provided without
assigned amount to collect the remainder. The `sendall` call will
never create change. The call has a `send_max` option that changes the
default behavior of spending all UTXOs ("no UTXO left behind"), to
maximizing the output amount of the transaction by skipping uneconomic
UTXOs. The `send_max` option is incompatible with providing a specific
set of inputs.
---
Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.
ACKs for top commit:
achow101:
re-ACK bb84b7145b31dbfdcb4cf0b9b6e612a57e573993
Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
|
|
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
|
|
misdocumented JSON
fc892c3a80091fbeaa2b5a6ec5bdaa31359b42de rpc: Fail to return undocumented or misdocumented JSON (MarcoFalke)
f4bc4a705addea3e60c3b69437913e6571df275d rpc: Add m_skip_type_check to RPCResult (MarcoFalke)
Pull request description:
This avoids documentation shortcomings such as the ones fixed in commit e7b6272b305386a264adf2c04b7bebfb8499070f, 138d55e6a0241f126916fde6ac9177c7e2a119c4, 577bd51a4b8de066466a445192c1c653872657e2, f8c84e047c61200fae4cc1d85688e113bf270409, 0ee9a00f90e81a6978b30bdb250a37cbfa6da022, 13f41855c5fedf11d4a2525f2f0dd214533e5e62, or faecb2ee0a0ad3eb8303cfc84a87dc02ec553c43
ACKs for top commit:
fanquake:
ACK fc892c3a80091fbeaa2b5a6ec5bdaa31359b42de - tested that this catches issue, i.e #24691:
Tree-SHA512: 9d0d7e6291bfc6f67541a4ff746d374ad8751fefcff6d103d8621c0298b190ab1d209ce96cfc3a0d4a6a5460a9f9bb790eb96027b16e5ff91f2512e40c92ca84
|
|
Don't extend shared ownership of all wallets to GetWalletForJSONRPCRequest scope.
|
|
The final step of send either produces a PSBT or the final transaction.
We extract these steps to a new helper function `FinishTransaction()` to
reuse them in `sendall`.
|
|
This will be reused in `sendall`, so we extract a method to prevent
duplication.
|
|
|
|
This will be reused in `sendall` so we extract it to avoid
duplication.
|
|
049003fe68a4183f6f20da16f58f10079d1e02df coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6adb6cbf9c87f04d3d667701905ef5c0a0 coinselection: Remove CInputCoin (Andrew Chow)
70f31f1a81710aa59e95770de9a84bf58cbce1e8 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57b79c664090f6a4e60d7bdfc9759ff4307 coinselection: Add effective value and fees to COutput (Andrew Chow)
f0821230b8de2eec21a869d1edf9e2b9f502de25 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e15c6deba1d9395a4da9341c9ebec6e8e5 wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5ad15ae56df56edee7ca9a202b52037889 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d1916e26e2a5d603edcdb7625463989d25b6 wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d3bb0d6e3ca55bcd23ce53e4fe413a9360 wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814bbd53736b79495072f3c9e05989a465e8 wallet: Store tx time in COutput (Andrew Chow)
46022953ee2e8113167bafd1fd48a383a578b13c wallet: Remove use_max_sig default value (Andrew Chow)
10379f007fd2c18f4cd24d0a0783d6d929f45556 scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db41e1718584aa2f30ff27f60ab0966de62 wallet: cleanup COutput constructor (Andrew Chow)
Pull request description:
While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.
`COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.
ACKs for top commit:
ryanofsky:
Code review ACK 049003fe68a4183f6f20da16f58f10079d1e02df, just adding comments and removing == operators since last review
w0xlt:
reACK 049003f
Xekyo:
reACK 049003fe68a4183f6f20da16f58f10079d1e02df
Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15
|
|
|
|
`sqlite.h`
39b1763730177cd7d6a32fd9321da640b0d65e0e Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)
Pull request description:
Contributes to #21005.
The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.
Notes:
* My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
* GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
* My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.
ACKs for top commit:
achow101:
ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e
ryanofsky:
Code review ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review
Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
|
|
Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.
|