wallet fix missing amount with fee exceeds balance #2761
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To successfully call the `capnp_generate_cpp()` function, the `libmultiprocess` build system must be provided with paths to the native `capnp` and `capnpc-c++` tools.
fa12706 Reject invalid rpcauth formats (MacroFake) Pull request description: This was added in commit 438ee59, but I couldn't determine if it was intentional. One reason to accept `foo:bar:baz` over `foo:bar$baz` is that `$` may be eaten by the shell. Though, I don't think many users pass `rpcauth` via the shell. Also it should be easy to avoid by passing `'-rpcauth=foo:bar$baz'` or `"-rpcauth=foo:bar\$baz"`. Can be tested with the added test. ACKs for top commit: pk-b2: ACK fa12706 Tree-SHA512: 9998cbb295c79f7b0342bf86e1d3e5b5ab90851c627662ad6495b699a65a9035998173cf1debfd94325387faba184de683407b609fe86acdd8f6749157644441
… members noexcept e5485e8 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack) abc1ee5 validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack) Pull request description: along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench). A swap must not fail; when a class has a swap member function, it should be declared noexcept. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail ACKs for top commit: pk-b2: ACK bitcoin@e5485e8 w0xlt: ACK bitcoin@e5485e8 Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
this ensures bitcoind option help is the source of truth and also gives an example conf file for users to customize and copy to their data directory. closes bitcoin#10746
include instructions on how to run the script
fad0abf lint: Fix lint-circular-dependencies.py file list (MacroFake) Pull request description: currently in-tree files like `wallet/test/fuzz/coinselection.cpp` are missed. Also out-of-tree files like `test/data/bip341_wallet_vectors.json.h` or `qt/moc_qvaluecombobox.cpp` are included. Change the script to only use in-tree files. Also, change `'python3'` to `sys.executable`. ACKs for top commit: laanwj: Code review ACK fad0abf Tree-SHA512: baf150fbae6a7120b2692f2eaef6a7773f2681e1610f8776f8d2ae6736c74736502a505df080b2182880f753b90f94e76a1e365fb45185f46f0e4d5521ca8e86
fa753ab rpc: Move fee estimation RPCs to separate file (MacroFake) Pull request description: Fee estimation is generally used by wallets when creating txs. It doesn't have anything to do with creating or submitting blocks. ACKs for top commit: pk-b2: ACK bitcoin@fa753ab brunoerg: crACK fa753ab Tree-SHA512: 81e0edc936198a0baf0f5bfa8cfedc12db51759c7873bb0082dfc5f0040d7f275b35f639c6f5b86fa1ea03397b0d5e757c2ce1b6b16f1029880a39b9c3aaceda
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
-BEGIN VERIFY SCRIPT- git mv src/rpc/misc.cpp src/rpc/node.cpp sed -i 's@rpc/misc.cpp@rpc/node.cpp@g' $(git grep -l misc.cpp) sed -i 's,RegisterMiscRPCCommands,RegisterNodeRPCCommands,g' $(git grep -l RegisterMiscRPCCommands) -END VERIFY SCRIPT-
…s (feature_fee_estimation.py performance fix) a498acc test: MiniWallet: skip mempool check if `mempool_valid=False` (Sebastian Falbesoner) 01552e8 test: MiniWallet: always rehash after signing (P2PK mode) (Sebastian Falbesoner) Pull request description: MiniWallet's core method for creating txs (`create_self_transfer`) right now always executes the `testmempoolaccept` RPC to check for mempool validity or invalidity. In some test cases where we use MiniWallet to create a huge number of transactions this can lead to performance issues, in particular feature_fee_estimation.py where the execution time after MiniWallet usage (PR bitcoin#24817) doubled, see bitcoin#24828 (comment), bitcoin#24828 (comment). This PR mitigates this by skipping the mempool check if the parameter `mempool_valid` is set to `False`. As a preparatory commit, the test feature_csv_activation.py has to be adapted w.r.t. to rehashing of transactions, as we now hash all transactions immediately in `create_self_transfer` in order to get the txid (before we relied on the result of `testmempoolaccept`). On my machine, this decreases the execution time quite noticably: master branch: ``` $ time ./test/functional/feature_fee_estimation.py real 3m20.771s user 2m52.360s sys 0m39.340s ``` PR branch: ``` $ time ./test/functional/feature_fee_estimation.py real 2m1.386s user 1m42.510s sys 0m22.980s ``` Partly fixes bitcoin#24828 (hopefully). ACKs for top commit: danielabrozzoni: tACK a498acc Tree-SHA512: f20c358ba42b2ded86175f46ff3ff9eaefb84175cbd1c2624f44904c8d8888e67ce64d6dcbb26aabbf07906e6f5bdea40353eba9ae668618cadcfc517ef7201b
88044a1 Guard `#include <config/bitcoin-config.h>` (Hennadii Stepanov) Pull request description: A fix for builds when the `HAVE_CONFIG_H` macro is not defined. ACKs for top commit: Empact: Code Review ACK bitcoin@88044a1 Tree-SHA512: f2bf1693c7671d7113dccaf66ae34a84719d86cb3271fa18b36611deab93a48d787b3ccfbd735d3b763017d709971cb1151d8d7f30390720009e6e2a6275b5b0
…lowed by path append operators f64aa9c Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky) Pull request description: Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding. Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier. In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions. Motivation for this PR was just that I was experimenting with bitcoin#24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them. ACKs for top commit: hebasto: ACK f64aa9c Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
See PR 22278 for discussion. Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Note that `SplitString` doesn't support token compression, but in this case it does not matter as empty strings are already skipped anyways. Also removes split.hpp and classification.hpp from expected includes
Also removes boost/algorithm/string.hpp from expected includes
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
c2b2958 tidy: add readability-redundant-declaration (fanquake) Pull request description: ACKs for top commit: vincenzopalazzo: ACK bitcoin@c2b2958 jonatack: Review-only ACK c2b2958 Tree-SHA512: 992dd81f9d0c511efcd8d9d1a8c05fc1401b854272f28f7f31ca0922164ddd7d7c01bfcf5ca268472b5d68969137110f5c0844a52938d294750584e1a948a874
…ckfilterheaders` (REST) d1bfe5e test: add coverage for invalid requests for `blockfilterheaders` (brunoerg) Pull request description: This PR adds test coverage for invalid requests (`Invalid hash` and `Unknown filtertype`) for `/blockfilterheaders` in REST functional test. ACKs for top commit: jonatack: ACK d1bfe5e vincenzopalazzo: ACK bitcoin@d1bfe5e Tree-SHA512: 9ab7efe7131296577c60642f95921799cf1dbae9c2aaea6752d2ac9f35a1bcc72b9d742a146c314f82fe1848190a80c88836ab78fc28773ed12e97fa327828e7
…ThreadedSchedulerClient fa4652c Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake) Pull request description: Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference. All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines. ACKs for top commit: pk-b2: ACK bitcoin@fa4652c jonatack: Code review ACK fa4652c rebased to master, debug build, unit tests vincenzopalazzo: ACK bitcoin@fa4652c Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
…ename misc.cpp fa758f9 scripted-diff: Rename rpc/misc.cpp to rpc/node.cpp (MacroFake) fa87eb8 rpc: Move output script RPCs to separate file (MacroFake) Pull request description: RPCs handling output scripts (addresses, scriptPubKeys, and output script descriptors) should not be placed in a file called `misc.cpp`, so move them out, then rename `misc.cpp`. ACKs for top commit: pk-b2: ACK bitcoin@fa758f9 vincenzopalazzo: ACK bitcoin@fa758f9 Tree-SHA512: 0cf8b5b8456361015513e93d3e604ea07d998dd578415b1d0e2918fb401fc44547fc1bb80b7c33c2086f6268e7b8f59837d2955f57434f646ea7921f0158b32d
…dBlock()::start_time 4cb9d21 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack) Pull request description: Suggested in bitcoin#25016 (comment), the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time. See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and bitcoin#22278 for related discussion, and bitcoin#25040 for a similar example. ACKs for top commit: MarcoFalke: review ACK 4cb9d21 Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
…Linux hosts c0f5cc1 build: Fix `libmultiprocess` cross-compiling to Linux hosts (Hennadii Stepanov) Pull request description: To successfully call the [`capnp_generate_cpp()`](https://github.com/chaincodelabs/libmultiprocess/blob/d576d975debdc9090bd2582f83f49c76c0061698/CMakeLists.txt#L45) function, the `libmultiprocess` build system must be provided with paths to the native `capnp` and `capnpc-c++` tools. This [comment](bitcoin#24387 (comment)) points the same: > I think `packages/libmultiprocess.mk` probably needs to be passing a `-DCAPNP_EXECUTABLE=.../depends/arm-linux-gnueabihf/native/bin/capnp` argument to cmake. Also the package should have dependencies on both `capnp` and `native_capnp`. Fixes bitcoin#24387. ACKs for top commit: ryanofsky: Code review ACK c0f5cc1 Tree-SHA512: 2986d8bf98d2761eceba21b1897145c5185a0922d4c2084e8812d4d07dc94237e5c2809036641c4f7c491a3414727fff328cba91ce138b89e37ec5cba61d8f61
…if the label is not in the address book.
Qt 5: - no behavior change Qt 6: - this change avoids sending a duplicated `QEvent::Quit`
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.
Since the removal of NODISCARD in 81d5af4, the only attributes def is LIFETIMEBOUND, and it's included in many more places that it is used. This removes all includes which do not have an associated use of LIFETIMEBOUND, and adds it to the following files, due to their use of the same: * src/validationinterface.h * src/script/standard.h
…ption methods a63b60f refactor: Add OptionsModel getOption/setOption methods (Ryan Ofsky) Pull request description: This is a trivial change which is needed as part of bitcoin#602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from bitcoin#602 because it causes a lot of rebase conflicts (any time there is a GUI options change). This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1 ACKs for top commit: vasild: ACK a63b60f furszy: Code review ACK a63b60f promag: Code review ACK a63b60f. Tree-SHA512: 1d99a1ce435b4055c1a38d2490702cf5b89bacc7d168c9968a60550bfd9f6dace649d5e98699de68d6305f02d5d1e3eb7e177ab578b98b996dd873b1df0ed236
…in OpenBSD build guide 9ecb0a3 doc: remove passing `--disable-external-signer` in OpenBSD build guide (Sebastian Falbesoner) Pull request description: Since we have a Boost.Process usage check in the build system (bitcoin#24254, commit abc057c), passing the option `--disable-external-signer` explicitly is not needed anymore on OpenBSD; the configure script will automatically detect that including `<boost/process.hpp>` leads to a compile error and disable external signer support accordingly: ``` $ ./configure MAKE=gmake ... checking whether Boost.Process can be used... no ... Options used to compile and link: external signer = no ... $ ./configure --enable-external-signer MAKE=gmake ... checking whether Boost.Process can be used... no configure: error: External signing is not supported for this Boost version ``` The PR basically reverts bitcoin#22335 but keeps the part mentioning that external signer support is not available on OpenBSD. Also bumps the guide to version 7.1 (released [about a month ago](https://www.openbsd.org/71.html)), where I could verify that the instructions are still accurate. ACKs for top commit: fanquake: ACK 9ecb0a3 Tree-SHA512: a5f7e89a5a78f062a06e0047802c42ad49d89e0f0afb963886caa684966ea2e9c8a660320eedd98a5aa5eee0a9c2bb8bf7f5772338c4b49738a69c00e9367a15
…reApplication::quit()` with `QCoreApplication::exit(0)` 252f363 qt: Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)` (Hennadii Stepanov) Pull request description: ### Qt 5: - no behavior change. See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qcoreapplication.cpp?h=5.15#n2012: ```cpp void QCoreApplication::quit() { exit(0); } ``` ### Qt 6: - this change avoids sending a duplicated `QEvent::Quit` We use `QEvent::Quit` to [handle](bitcoin-core/gui#547) macOS dock menu events. Qt 6 uses `QEvent::Quit` more [widely](qt/qtbase@89f7a27). We do not want a duplicated `QEvent::Quit` which fires `Assert(node.args);` in the [`Shutdown()`](https://github.com/bitcoin-core/gui/blob/d1b3dfb275fd98e37cfe8a0f7cea7d03595af2e8/src/init.cpp#L200) function. ACKs for top commit: promag: Code review ACK 252f363. Tree-SHA512: 6a04cbcf523c0375158a59b29afadf18da99738c8db8b8728f99319a8cdc10806d2f06dc5a7d3b8b0e1a5f1711be778a75d4ecdefef7cf66e26ae2848f7f57db
71a8dbe refactor: Remove defunct attributes.h includes (Ben Woosley) Pull request description: Since the removal of NODISCARD in 81d5af4, the only attributes.h def is LIFETIMEBOUND, and it's included in many more places that it is used. This removes all includes which do not have an associated use of LIFETIMEBOUND, and adds it to the following files, due to their use of the same: * src/validationinterface.h * src/script/standard.h See also bitcoin#20499. Top commit has no ACKs. Tree-SHA512: f3e10a5cda5ab78371b77b702f4a241ff69d490a16cc6059f1a4202b97c584accdbc951cc7b6120eae94bee3b9249e9117b45cf6ed1a5228ca23b5638fcf7b7b
Instead of using permissions from the local file system, which might depend on the umask, directly check the permissions from git's metadata.
…esses were found in the address book baa3ddc doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy) 8897a21 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 bitcoin#23662, coming from comment bitcoin#23662 (review). 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 baa3ddc theStack: re-ACK baa3ddc w0xlt: ACK bitcoin@baa3ddc Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
6fbb0ed 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 6fbb0ed Xekyo: re-ACK 6fbb0ed w0xlt: Code Review ACK bitcoin@6fbb0ed furszy: Looks good, have been touching this area lately, code review ACK 6fbb0ed. Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
908fb7e test: Use permissions from git in `lint-files.py` (laanwj) 48d2e80 test: Don't use shell=True in `lint-files.py` (laanwj) Pull request description: Improvements to the `lint-files.py` script: - Avoid use of `shell=True`. - Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`. (what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally). ACKs for top commit: vincenzopalazzo: re-tACK bitcoin@908fb7e Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
And connect it to 'AvailableCoins' method
Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.
…tPubKey) This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.
…only and using the 'AvailableCoins' retrieved total_amount.
'createTransaction' returns 'nFeeRequired' only if the transaction creation process succeeded (internally, it sets the fee into the 'CreatedTransactionResult' return struct only at the end of the creation process). So the "AmountWithFeeExceedsBalance" error now needs to be re-written differently inside `createTransaction`.
…ransaction fee is included error".
…le balance only for the fee.
|
not here.. |
furszy
changed the title
panleone
pushed a commit
to panleone/PIVX
that referenced
this pull request
Nov 5, 2024
* use AssertLockHeld(cs) instead of relying on comments * actually use `clsig` in `EnforceBestChainLock()` * fix log output in `EnforceBestChainLock()` * drop comments
panleone
pushed a commit
to panleone/PIVX
that referenced
this pull request
Nov 5, 2024
* use AssertLockHeld(cs) instead of relying on comments * actually use `clsig` in `EnforceBestChainLock()` * fix log output in `EnforceBestChainLock()` * drop comments
Fuzzbawls
added a commit
that referenced
this pull request
Nov 9, 2024
901976a Stop tracking interested/participating nodes and send/announce to MNAUTH peers (#2798) (Alexander Block) 59dfdc2 update bestChainLockWithKnownBlock in AcceptedBlockHeader (Alessandro Rezzi) bd0a3f8 Introduce "qsendrecsigs" to indicate that plain recovered sigs should be sent (#2783) (Alexander Block) 846fd9a Make LLMQ/InstantSend/ChainLocks code less spammy (#2781) (Alexander Block) 1a52ad0 Fix LogPrintf call in ::DoInvalidateBlock (Alessandro Rezzi) e3c5eef Multiple fixes/refactorings for ChainLocks (#2765) (Alexander Block) 9dda5d2 Implement LLMQ based InstantSend (#2735) (Alexander Block) 149e3e3 Various small cleanups (#2761) (UdjinM6) cc7450d Use ReleaseNodeVector and CopyNodeVector in PIVX specific code (Alessandro Rezzi) ed463a6 Do not hold cs_vNodes in CSigSharesManager::SendMessages() for too long (#2758) (UdjinM6) cd1809f Implement persistence for LLMQ based InstantSend (#2756) (Alexander Block) 924ee8f Don't be too harsh for invalid CLSIGs (#2742) (Alexander Block) f255314 Fix banning when local node doesn't have the vvec (#2739) (Alexander Block) Pull request description: each commit backports a different PR. The PR number is in the commit message. There are also 3 extra (trivial) commits added by me ACKs for top commit: 901976a Duddino: utACK 901976a Liquid369: uTACK 901976a Tree-SHA512: 70b53ae3fc014c23964cae3452f6c8270d96b3b375c0f172e04039ba6f3b537d56c38677e5e4005c8b810a82b447a64fc978c2d63344b9e9736ceff7db162a18
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.