Skip to content

Conversation

ryanofsky
Copy link
Contributor

I found this useful while debugging silent conflict between #10102 and #27469 recently

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 27, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK furszy, ishaanam, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27307 (wallet: track mempool conflicts with wallet transactions by ishaanam)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dc1520b

I'm slightly more inclined to encapsulate the string representation into a toString() per struct so we don't have to touch different functions when adding a new tx state. But.. that could be just me. Not blocking at all.

diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
--- a/src/wallet/transaction.h	(revision 42056e6a2944b3f61aed376b3317551ca1241971)
+++ b/src/wallet/transaction.h	(date 1695826678557)
@@ -29,10 +29,13 @@
     int position_in_block;
 
     explicit TxStateConfirmed(const uint256& block_hash, int height, int index) : confirmed_block_hash(block_hash), confirmed_block_height(height), position_in_block(index) {}
+
+    std::string toString() const { return strprintf("Confirmed (block=%s, index=%i)", confirmed_block_hash.ToString(), position_in_block); }
 };
 
 //! State of transaction added to mempool.
 struct TxStateInMempool {
+    std::string toString() const { return strprintf("InMempool"); }
 };
 
 //! State of rejected transaction that conflicts with a confirmed block.
@@ -41,6 +44,8 @@
     int conflicting_block_height;
 
     explicit TxStateConflicted(const uint256& block_hash, int height) : conflicting_block_hash(block_hash), conflicting_block_height(height) {}
+
+    std::string toString() const { return strprintf("Conflicted (block=%s)", conflicting_block_hash.ToString()); }
 };
 
 //! State of transaction not confirmed or conflicting with a known block and
@@ -51,6 +56,8 @@
     bool abandoned;
 
     explicit TxStateInactive(bool abandoned = false) : abandoned(abandoned) {}
+
+    std::string toString() const { return strprintf("Inactive (abandoned=%i)", abandoned); }
 };
 
 //! State of transaction loaded in an unrecognized state with unexpected hash or
@@ -62,6 +69,8 @@
     int index;
 
     TxStateUnrecognized(const uint256& block_hash, int index) : block_hash(block_hash), index(index) {}
+
+    std::string toString() const { return strprintf("Unrecognized (block=%s, index=%i)", block_hash.ToString(), index); }
 };
 
 //! All possible CWalletTx states
@@ -109,6 +118,11 @@
     }, state);
 }
 
+//! Return TxState as a string for logging or debugging.
+static inline std::string TxStateString(const TxState& state)
+{
+    return std::visit([](const auto& s) { return s.toString(); }, state);
+}
 
 /**
  * Cachable amount subdivided into watchonly and spendable parts.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
@ryanofsky
Copy link
Contributor Author

re: #28544 (review)

I'm slightly more inclined to encapsulate the string representation into a toString() per struct so we don't have to touch different functions when adding a new tx state. But.. that could be just me. Not blocking at all.

Agree this is nicer. It also lets the function work with SyncTxState variants, not just TxState.

Updated dc1520b -> 8a553c9 (pr/statestr.1 -> pr/statestr.2, compare) with suggestion

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code ACK 8a553c9

@ishaanam
Copy link
Contributor

utACK 8a553c9

@maflcko maflcko requested a review from achow101 October 17, 2023 08:41
@achow101
Copy link
Member

ACK 8a553c9

@DrahtBot DrahtBot removed the request for review from achow101 October 17, 2023 19:20
@achow101 achow101 merged commit fbcf102 into bitcoin:master Oct 17, 2023
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023
… and logging

8a553c9 wallet: Add TxStateString function for debugging and logging (Ryan Ofsky)

Pull request description:

  I found this useful while debugging silent conflict between bitcoin#10102 and bitcoin#27469 recently

ACKs for top commit:
  ishaanam:
    utACK 8a553c9
  achow101:
    ACK 8a553c9
  furszy:
    Code ACK 8a553c9

Tree-SHA512: 87965c66bcb59a21e7639878bb567e583a0e624735721ff7ad1104eed6bb9fba60607d0e3de7be3304232b3a55f48bab7039ea9c26b0e81963e59f9acd94f666
@bitcoin bitcoin locked and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants