Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Up to CTransactionRef point back ports to do list. #1726

Closed
19 of 21 tasks
furszy opened this issue Jul 3, 2020 · 1 comment
Closed
19 of 21 tasks

Up to CTransactionRef point back ports to do list. #1726

furszy opened this issue Jul 3, 2020 · 1 comment

Comments

@furszy
Copy link

furszy commented Jul 3, 2020

This issue tracker is for tracking some specific back ports to get up to the final migration to CTransactionRef point.

Will try to continue adding more as them appear down the back porting rabbit hole. Most likely there are more PRs in-between the ones above that need to be back ported too.

Everyone is more than welcome to join this effort :) . Just pick one on the list and start doing it.

random-zebra added a commit that referenced this issue Aug 18, 2020
3aded91 [Trivial] Temporarily comment-out priority check in policyestimator test (random-zebra)
245c3df [Tests] Enable smartfee test in feature_fee_estimation.py (random-zebra)
a06e9ff Pass reference to estimateSmartFee and cleanup whitespace (Suhas Daftuar)
9df303d Expose RPC calls for estimatesmart functions (furszy)
826aaaa add estimateSmartFee to the unit test (Alex Morcos)
e34b723 EstimateSmart functions consider mempool min fee (Alex Morcos)
649b4fc Increase success threshold for fee estimation to 95% (Alex Morcos)
4893a7c Change wallet and GUI code to use new smart fee estimation calls. (furszy)
e8fc55a Add smart fee estimation functions (Alex Morcos)

Pull request description:

  Another back port, coming from dashpay#6134 .
  > When automatically adding a fee estimate to a transaction and in the event no estimate is available for the desired confirmation target, it makes more sense to default to a fee estimate for a worse confirmation target rather than the hard coded minimum.
  >

  TODO: Left to back port smartfees.py.

  We need it to be able to move forward in #1726

ACKs for top commit:
  Fuzzbawls:
    utACK 3aded91
  random-zebra:
    ACK 3aded91 and merging...

Tree-SHA512: 6f52d29267da0b20e54ed680743b341c1e616986dabf72916499cf5936d73d552bab907038e83b65625d6bf9dbd0adbb1e1300a15622d94908f54e6fe270f2f6
furszy added a commit that referenced this issue Sep 24, 2020
911e31c Make CBlock::vtx a vector of shared_ptr<CTransaction> (furszy)
9d27b75 Add deserializing constructors to CTransaction and CMutableTransaction (Pieter Wuille)
5f90940 Add serialization for unique_ptr and shared_ptr (Pieter Wuille)

Pull request description:

  Inspired on bitcoin#9125 (with many more adaptations), preparation for bitcoin#8580. Ant work 🐜

  Establishing the bases for the work, we will need to continue migrating `CTransaction` to `std::shared_ptr<const CTransaction>` where is possible. Example bitcoin#8126 (can find more in #1726 list).

  TODO:
  * back port final `CTransactionRef` implementation commit.

  EDIT:
  This is now depending on #1816 .

ACKs for top commit:
  random-zebra:
    ACK 911e31c
  Fuzzbawls:
    ACK 911e31c

Tree-SHA512: 61d937aba7dce4ac0867496e7f81ae8351dcdd60b4e72c4f0ed58152a7e556bf455836c766bc97bbca331227e5deed92fa5ce609fa63bb9cb71600b430dc4405
furszy added a commit that referenced this issue Jan 23, 2021
9b9c616 Fix missing zapwallettxes mode in wallet_hd.py functional test (furszy)
d6d0ad9 [logs] fix zapwallettxes startup logs (John Newbery)
006c503 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery)
c6d45c6 Adapting and connecting mempool_persist.py functional test to the test runner. (furszy)
4f26a4e Control mempool persistence using a command line parameter. (John Newbery)
5d949de [Qt] Do proper shutdown (Jonas Schnelli)
e60da98 Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
c0a0e81 Moving TxMempoolInfo tx member to CTransactionRef (furszy)
f0c2255 Add mempool.dat to doc/files.md (furszy)
8e52226 Add DumpMempool and LoadMempool (Pieter Wuille)
44c635d Add AcceptToMemoryPoolWithTime function (Pieter Wuille)
6bbc6a9 Add feedelta to TxMempoolInfo (Pieter Wuille)
9979f3d [mempool] move removed and conflicts transaction ref list to vector. (furszy)
4f672c2 Make removed and conflicted arguments optional to remove (Pieter Wuille)
e51c4b8 Bypass removeRecursive in removeForReorg (Pieter Wuille)
54cf7c0 Get rid of CTxMempool::lookup() entirely (furszy)
35bc2a9 Finished the switch CTransaction storage in mempool to CTransactionRef and introduced the timeLastMempoolReq coming from btc#8080. (furszy)
d10583b An adapted version of btc@b5599147533103efea896a1fc4ff51f2d3ad5808 (furszy)
cb4fc6c An adapted version of btc@ed7068302c7490e8061cb3a558a0f83a465beeea (furszy)
9645775 Split up and optimize transaction and block inv queues (furszy)
68bc68f Don't do mempool lookups for "mempool" command without a filter (furszy)
7624823 mapNextTx: use pointer as key, simplify value (furszy)
191c62e Return mempool queries in dependency order (Pieter Wuille)
23c9f3e Eliminate TX trickle bypass, sort TX invs for privacy and priority. (furszy)
6ebfd17 tiny test fix for mempool_tests (Alex Morcos)
8c0016e Check all ancestor state in CTxMemPool::check() (furszy)
91c6096 Add ancestor feerate index to mempool (Suhas Daftuar)
64e84e2 Add ancestor tracking to mempool  This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). (furszy)
8325bb4 Fix mempool limiting for PrioritiseTransaction Redo the feerate index to be based on mining score, rather than fee. (furszy)
1fa40ac Remove work limit in UpdateForDescendants()  The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with.  This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. (furszy)
ba32375 Rename CTxMemPool::remove -> removeRecursive  remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter (furszy)
c30fa16 CTxMemPool::removeForBlock now uses RemoveStaged (furszy)

Pull request description:

  Ending up 2020 with a large PR :).

  Included a good number of performance and privacy improvements over the mempool and inv processing/sending areas + added mempool cache dump/load.
  Almost finishing with #1726 work, getting closer to 9725, and getting closer to be able to decouple the message processing thread <-> wallet and the wallet <-> GUI locks dependencies (which will be another long story.. but well, step by step).

  The final goal is clear, a much faster syncing process using pivx-qt (a good speed up for pivxd as well), smoother visual navigation when big wallets are syncing, less thread synchronization issues, among all of the improvements that will be coming with the backports adaptations.

  Adapted the following PRs to our sources:

  bitcoin#7062 —> only eb30666.
  bitcoin#7174 —> complete.
  bitcoin#7562 —> only c5d746a.
  bitcoin#7594 —> complete.
  bitcoin#7840 —> complete.
  bitcoin#7997 —> complete.
  bitcoin#8080 —> complete
  bitcoin#8126 —> except e9b4780 (we don't have the mapRelay) and c2a4724 (we don't have the relay expiration vector).
  bitcoin#8448 —> complete
  bitcoin#8515 —> complete
  bitcoin#9408 —> complete
  bitcoin#9966 —> complete
  bitcoin#10330 —> complete

ACKs for top commit:
  random-zebra:
    ACK 9b9c616
  Fuzzbawls:
    ACK 9b9c616

Tree-SHA512: 186bd09bbb19b55ec0d46dc27291663a0df2d60d8238c661a8c9b8cbf3677b6f8a92fa56bd7346a3cb5293712eeccc5d6542ee3c441d35a4f61e7d12e2ce489a
furszy added a commit that referenced this issue Jan 30, 2021
…llet

98d770f CScheduler boost->std::function, use millisecs for times, not secs (Matt Corallo)
67e068c Remove now unneeded ChainTip signal (furszy)
bcdd3e9 Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. (furszy)
b799070 Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo)
d77244c Remove dead-code tracking of requests for blocks we generated (Matt Corallo)
10ccbbf Hold cs_wallet for whole block [dis]connection processing (Matt Corallo)
1a45036 fix compiler warning member functions not marked as 'override' (furszy)
d3867a7 An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 (Matt Corallo)
f5ac648 [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure (furszy)
8704d9d Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo)
6dcb6b0 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo)
50d3e0e Handle conflicted transactions directly in ConnectTrace (furszy)
27fb897 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo)
60329da Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo)
e7c2789 Include missing #include in zmqnotificationinterface.h (Matt Corallo)
1b396b8 Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler (furszy)
4cb5820 Better document usage of SyncTransaction (Alex Morcos)
21be4e2 Introduce MemPoolConflictRemovalTracker (Alex Morcos)
7326acb mempool: add notification for added/removed entries (Wladimir J. van der Laan)
a8605d2 Clean up tx prioritization when conflict mined (Casey Rodarmor)
e7db9ff remove internal tracking of mempool conflicts for reporting to wallet (Alex Morcos)
76c72c6 remove external usage of mempool conflict tracking (Alex Morcos)
ef429af tests: Stop node before removing the notification file (furszy)
15a21c2 tests: write the notification to different files to avoid race condition (Chun Kuan Lee)
466e97a [Wallet] Simplify InMempool (furszy)
85e18f0 Rename FlushWalletDB -> CompactWalletDB, add function description (Matt Corallo)
00f36ea Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (Matt Corallo)

Pull request description:

  Revamped the validation interface interaction with the wallet, encapsulating and improving the mempool and block signaling and each of the wallet signals handler.
  Restructured the Sapling witnesses and nullifiers update under the new signals.
  Solved several bugs that found on the way as well (Check each commit description).

  This PR concludes with #1726 long road :). Pushing our repository around 2 years ahead in btc time in the mempool and validation interface areas (without including RBF).
  The new validation -> wallet interaction architecture will enable further, and much more user facing important, improvements for the syncing process, overall software responsiveness and multithreading locking issues.

  Adapted backports:
  bitcoin#6464 --> Always clean up manual transaction prioritization (mempool)
  bitcoin#9240 --> Remove txConflicted (mempool)
  bitcoin#9371 --> Notify on removal (mempool)
  bitcoin#9605 --> Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (walletdb)
  bitcoin#9725 --> CValidationInterface Cleanups (wallet, validation and validation interface)

ACKs for top commit:
  random-zebra:
    utACK 98d770f
  Fuzzbawls:
    utACK 98d770f

Tree-SHA512: 84c86567c2bff36b859b2ae73c558956a70dff0fffb4f73208708d92165b851bf42d35246410238c66c7a4b77e5bf51ec93885234a75fa48901fd182d2f70a28
@furszy
Copy link
Author

furszy commented Feb 1, 2021

This long to do list is completed :). Closing the card.

@furszy furszy closed this as completed Feb 1, 2021
random-zebra added a commit that referenced this issue Mar 12, 2021
ca3edc5 GUI: Update worker type if task already exist. (furszy)
a1390c3 wallet balances, cache total delegated balance and calculate it only once. (furszy)
02eb781 GUI: settings information, do not update block num and block hash if the screen is not visible. (furszy)
cbc5021 Masternode-sync read only function to get the "IsBlockchainSynced" state. (furszy)
ef77fdf GUI: topbar, removing not used block source. (furszy)
e9c160a wallet: single loop to calculate the currently required balances. (furszy)
32538ae wallet: Disallow abandon of conflicted txes (furszy)
62cd35f GUI: MN model, remove now unneeded cs_main locks. (furszy)
5658844 wallet: removing unnecessary `mempool.cs` lock from ReacceptWalletTransactions() (furszy)
63f3fa7 GUI: add missing qt metatype declarations. (furszy)
bc76186 wallet model: update delay increased to 5 seconds. (furszy)
3fb3f34 GUI dashboard: do not update the staking filter if it's not needed. (furszy)
7cb8276 init: move "Done loading" message and rpc warm up finished after the wallet post initialization process (furszy)
8762e81 Refactoring with QString::toNSString (Hennadii Stepanov)
69b3330 GUI: txmodel, do not lock cs_wallet if no wtx status update is needed. (furszy)
b4ab286 GUI: dashboard, charts update delay time increased for IBD. (furszy)
0f197ca Wallet interface: do not load not used cold staking balances. (furszy)
dee4224 Wallet:GetLockedCoins() loop only over the locked coins, not over the entire wallet txes map. (furszy)
0a61f7f GUI: cold staking, do not refresh delegations if the screen is not visible. (furszy)
c2d66d0 GUI: cold staking screen, do not initialize coin control dialog at startup. (furszy)
44d9cbe GUI: dashboard screen, remove stakes filter source model if chart is not being presented. (furszy)
6e49a11 GUI: send screen, initialize coinControlDialog view only when it's being called. (furszy)
379e5f2 GUI: send screen, move refresh amounts calculation to a background worker thread. (furszy)
76bc08b GUI: balance polling update moved to a background worker thread. (furszy)
b73500a wallet:BlockConnected do not lock cs_wallet for its entire process. (furszy)
208a292 wallet: remove last cs_main locks from every signal handler function :) . (furszy)
9720579 SSPKM: remove redundant ReadBlockFromDisk from IncrementNoteWitnesses. (furszy)
4ccec74 wallet: Add IsSpent() cs_wallet lock assertion. (furszy)
e0a0d2d sapling_wallet_tests: locking order refactor, solving inconsistent orders for cs_main and cs_wallet. (furszy)
c69e7e8 Adapt sapling_wallet_listreceived.py to the new wtx confirmation structure. (furszy)
c4952a2 Wallet::CreateWalletFromFile guard direct chainActive access (furszy)
f889dcb test : updating wallet's last block processed manually. (furszy)
45c9471 wallet: split CheckTXAvailability in two. (furszy)
5109c8d Wallet: remove cs_main from IsSaplingSpent() and GetFilteredNotes() (furszy)
a7f6ab1 Wallet: remove cs_main requirement from RelayWalletTransaction and FundTransaction. (furszy)
1e7ffc2 Wallet: remove cs_main requirement from CreateCoinStake (furszy)
2e7cdb2 Wallet: added max value out filter for AvailableCoins. (furszy)
b9220f4 Wallet: Do not add P2CS utxo to autocombine flow and discard them later. (furszy)
59ed47d Wallet: remove cs_main lock from AutoCombineDust plus a redundant maturity check. (furszy)
fcc4c83 Move AutoCombineDust functionality to the wallet background thread (furszy)
fcb20c2 GUI: remove cs_main lock dependency from CWalletModel::isSpent, CWalletModel::isLockedCoin, CWalletModel::lockCoin, CWalletModel::unlockCoin (furszy)
65fbad1 GUI: remove cs_main lock dependency from transaction model update. (furszy)
0b61857 GUI: fix coinstake tx ordering. (furszy)
33588fe GUI: Remove cs_main lock and chain dependency from transaction update status (furszy)
3dede64 GUI: Remove cs_main lock from balance polling timer (furszy)
5e06330 Removed IsFinalTx() cs_main lock requirement. (furszy)
1bd97ca wallet::MarkConflicted remove blockIndex and there by cs_main lock dependency. (furszy)
239d6a2 wallet: remove the now unneeded cs_main locks (furszy)
1386ab7 Use CWallet::m_last_block_processed_height in GetDepthInMainChain (furszy)
9adeb61 Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (furszy)
8aa2d31 Add block_height field in struct Confirmation (furszy)
4405ac0 Replace CWalletTx::SetConf by Confirmation initialization list (furszy)
6320efb Update wallet last processed index in every unit test that needs it (furszy)
4957051 wallet: cache block hash, height and time inside the wallet. (furszy)
33f4788 wallet: refactor GetDepthInMainChain to not return the block index. (furszy)
e7c8ca6 wallet: remove unused IsInMainChain method (furszy)
9e51a48 Add a test wallet_reorgsrestore (Antoine Riard)
370c200 wallet: simplifying pindexRescan set + added an AssertLockHeld on FindForkInGlobalIndex (furszy)
46f0e30 Fixing reindex problem, use mapBlockIndex.find() and not mapBlockIndex[] (furszy)
da78039 [Wallet] Adapting TransactionAddedToMempool and BlockDisconnected to the new wtx confirmation status (furszy)
ff04fa6 Modify wallet tx status if has been reorged out (furszy)
f7baeaf Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
8d8928e Encapsulate tx status in a Confirmation struct (Antoine Riard)

Pull request description:

  This is the conclusion of a deep deep rabbit hole: #1726, #2150, #2082, #2118, #2150, #2179, #2191, #2192, #2195, #2201, #2203..

  Effectively removing every `cs_main` lock from the wallet and GUI processing threads. Completely decoupling the wallet state and the visual interface update procedures from the main message and validation handler thread.

  Meaning that the messages & validation handler thread will be largely more active, accepting and verifying way more data in less time, not being affected by several other threads accessing to the main critical section. And, at the same time, the wallet and GUI worker threads will be able to perform and process their tasks concurrently, without waiting for the `cs_main` mutex acquisition.
  Improving the overall software performance, the GUI responsiveness and decreasing the synchronization time.

  To make this possible, the wallet is maintaining in memory a view of the chain and updating it only via the validation interface signals. Using the view to perform all of the chain related calculations.

ACKs for top commit:
  Fuzzbawls:
    All good now, ACK ca3edc5
  random-zebra:
    ACK ca3edc5 and merging...

Tree-SHA512: 6d4268730941822942b0df0aab200683a1fabaf6801618cf34955b43b29bc2beb694567635f731a724abf7b73cec3edfe506e0428de6710c0fcf603613f5614a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant