| Age | Commit message (Collapse) | Author |
|---|
|
|
|
heights with asserts, comments, and refactoring
f06016d77d848133fc6568f287bb86b644c9fa69 wallet: Add asserts to detect unset transaction height values (Ryan Ofsky)
262a78b13365e5318741187fde431c4974539494 wallet, refactor: Add CWalletTx::updateState function (Ryan Ofsky)
Pull request description:
Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in #28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights.
This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in #28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced.
ACKs for top commit:
achow101:
ACK f06016d77d848133fc6568f287bb86b644c9fa69
Sjors:
utACK f06016d77d848133fc6568f287bb86b644c9fa69
furszy:
Code review ACK f06016d
Tree-SHA512: 82657c403724d60354f7676b53bcfcc95bdc5864e051a2eb8bfad09d8ad35615393b2d6b432b46f908def9be37bebded3a55ec9ae19e19371d35897fe842c92e
|
|
Then, use the compiler warnings to create copies only where needed.
Also, fix iwyu includes while touching the includes.
|
|
940a49978c70453e1aaf2c4a0bcb382872b844a5 Use type-safe txid types in orphanage (dergoegge)
ed70e6501648466b9ca91a39b83775363e9a726d Introduce types for txids & wtxids (dergoegge)
cdb14d79e809bf7d1612b21b554a9fcfb2ab1c91 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
stickies-v:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
hebasto:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
BrandonOdiwuor:
re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
|
|
after migration
4814e4063e674ad9b0a5c7e56059cd6a2bf9b764 test: Check tx metadata is migrated to watchonly (Andrew Chow)
d616d30ea5fdfb897f8375ffd8b9f4536ae7835b wallet: Reload watchonly and solvables wallets after migration (Andrew Chow)
118f2d7d70b584eee7b89e58b5cd2d61c59a9bbf wallet: Copy all tx metadata to watchonly wallet (Andrew Chow)
9af87cf3485ce3fac553a284cde37a35d1085c25 test: Check that a failed wallet migration is cleaned up (Andrew Chow)
Pull request description:
Some incomplete/incorrect state as a result of migration can be mitigated/cleaned up by simply restarting the migrated wallets. We already do this for a wallet when it is migrated, but we do not for the new watchonly and solvables wallets that may be created. This PR introduces this behavior, in addition to creating those wallets initially without an attached chain.
While implementing this, I noticed that not all `CWalletTx` metadata was being copied over to the watchonly wallet and so some data, such as time received, was being lost. This PR fixes this as a side effect of not having a chain attached to the watchonly wallet. A test has also been added.
ACKs for top commit:
ishaanam:
light code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764
ryanofsky:
Code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764. Just implemented the suggested orderpos, copyfrom, and path set comments since last review
furszy:
ACK 4814e406
Tree-SHA512: 0b992430df9f452cb252c2212df8e876613f43564fcd1dc00c6c31fa497adb84dfff6b5ef597590f9b288c5f64cb455f108fcc9b6c9d1fe9eb2c39e7f2c12a89
|
|
No change in behavior, this just moves code which updates transaction state to
a new method so it can be used after offline processes such as wallet
migration.
|
|
When moving a tx to the watchonly wallet during migration, make sure
that all of the CWalletTx data follows it.
|
|
|
|
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
|
|
Move isminetype and isminefilter there this commit, add WalletPurpose type next
commit.
|
|
b0fa5989e1b77a343349bd36f8bc407f9366a589 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd78901124a48fd41a82a9557fcf3490191d wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)
Pull request description:
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes #14148
ACKs for top commit:
furszy:
ACK b0fa5989 with a not-blocking nit.
aureleoules:
reACK b0fa5989e1b77a343349bd36f8bc407f9366a589
ishaanam:
ACK b0fa5989e1b77a343349bd36f8bc407f9366a589
Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Both of these functions do almost the exact same thing. They can be
deduplicated so that their behavior matches except for the filtering
aspect. As this function will now always be called on wallet loading,
nNextResend will also always be initialized, so
wallet_resendwallettransactions.py is updated to account for that.
This also resolves a bug where ResendWalletTransactions would fail to
rebroadcast txs in insertion order thereby potentially rebroadcasting a
child transaction before its parent and causing the child to not
actually get rebroadcast.
Also names the combined function to ResubmitWalletTransactions as the
function just submits the transactions to the mempool rather than doing
any sending by itself.
|
|
|
|
|
|
Current CWalletTx state representation makes it possible to set
inconsistent states that won't be handled correctly by wallet sync code
or serialized & deserialized back into the same form.
For example, it is possible to call setConflicted without setting a
conflicting block hash, or setConfirmed with no transaction index. And
it's possible update individual m_confirm and fInMempool data fields
without setting an overall consistent state that can be serialized and
handled correctly.
Fix this without changing behavior by using std::variant, instead of an
enum and collection of fields, to represent sync state, so state
tracking code is safer and more legible.
This is a first step to fixing state tracking bugs
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
by adding an extra margin of safety that can prevent new bugs from being
introduced as existing bugs are fixed.
|
|
Follow-up to:
* commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced
posInBlock with confirm.nIndex.
|
|
9d0379cea6c164610d05287ae6dd4e66f35b92b3 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63a67fa020fb1ef527b9095a35ab77a5 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5bc997f2de1f55ca7a9babc3d7619329 [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6c164610d05287ae6dd4e66f35b92b 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
|
|
std::from_chars(…) (C++17)
test: Add test cases for LocaleIndependentAtoi
fuzz: Assert legacy atoi(s) == LocaleIndependentAtoi<int>(s)
fuzz: Assert legacy atoi64(s) == LocaleIndependentAtoi<int64_t>(s)
|
|
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
|
|
Also, use a named cast for converting the atoi64() result into an
unsigned int type.
|
|
Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
this function is only called at one place and thus can be as well inlined.
|
|
Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
this function is only called at one place and thus can be as well inlined. Also,
avoid a duplicate lookup by using the find() method and dereference, instead of
calling count() and operator[].
|
|
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
|
|
|
|
This commit just moves functions without making any changes. It can be
reviewed with `git log -p -n1 --color-moved=dimmed_zebra`
Motivation for this change is to make wallet.cpp/h less monolithic and
start to make wallet transaction state tracking comprehensible so bugs
in
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
can be fixed safely without introducing new problems.
This commit moves wallet classes and methods that deal with transactions
out of wallet.cpp/.h into better organized files:
- transaction.cpp/.h - CWalletTx and CMerkleTx class definitions
- receive.cpp/.h - functions checking received transactions and computing balances
- spend.cpp/.h - functions creating transactions and finding spendable coins
After #20773, when loading is separated from syncing it will also be
possible to move more wallet.cpp/.h functions to:
- sync.cpp/.h - functions handling chain notifications and rescanning
This commit arranges receive.cpp and spend.cpp functions in dependency
order so it's possible to skim receive.cpp and get an idea of how
computing balances works, and skim spend.cpp and get an idea of how
transactions are created, without having to jump all over wallet.cpp
where functions are not in order and there is a lot of unrelated code.
Followup commit "refactor: Detach wallet transaction methods" in
https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and
tweaks function names and arguments to reflect new locations. The two
commits are split into separate PRs because this commit is more work to
maintain and less work to review, while the other commit is less work to
maintain and more work to review, so hopefully this commit can be merged
earlier.
|
