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

Also consider txindex for transactions in AlreadyHave() #3126

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

codablock
Copy link

This avoids many false negatives where TXs are announced to us which are
already mined and then were spent later.

This also avoids TXs going into the orphan map even though they were already mined.

@codablock codablock added this to the 14.1 milestone Sep 30, 2019
This avoids many false negatives where TXs are announced to us which are
already mined and then were spent later.
@codablock codablock force-pushed the pr_alreadyhave_optimizations branch from d0eb676 to 82b81d8 Compare September 30, 2019 13:32
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@@ -240,6 +240,10 @@ bool CBlockTreeDB::WriteBatchSync(const std::vector<std::pair<int, const CBlockF
return WriteBatch(batch, true);
}

bool CBlockTreeDB::HasTxIndex(const uint256& txid) {
Copy link
Member

Choose a reason for hiding this comment

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

I understand that you are trying to replicate surrounding code, but when adding new code we should follow code guidelines (ie move the { down to the next line)

@@ -121,6 +121,7 @@ class CBlockTreeDB : public CDBWrapper
bool ReadLastBlockFile(int &nFile);
bool WriteReindexing(bool fReindex);
bool ReadReindexing(bool &fReindex);
bool HasTxIndex(const uint256 &txid);
Copy link
Member

Choose a reason for hiding this comment

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

Again, I understand you are replicating surrounding code, but according to clang, it should be:

Suggested change
bool HasTxIndex(const uint256 &txid);
bool HasTxIndex(const uint256& txid);

Copy link

@nmarley nmarley left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6
Copy link

UdjinM6 commented Oct 1, 2019

After some discussion on Slack I'm leaning towards merging it as is in this specific case.

@UdjinM6 UdjinM6 merged commit b4aefb5 into dashpay:develop Oct 1, 2019
@codablock codablock deleted the pr_alreadyhave_optimizations branch November 18, 2019 14:33
codablock added a commit to codablock/dash that referenced this pull request Nov 18, 2019
This avoids many false negatives where TXs are announced to us which are
already mined and then were spent later.
codablock added a commit to codablock/dash that referenced this pull request Nov 19, 2019
This avoids many false negatives where TXs are announced to us which are
already mined and then were spent later.
MIPPL pushed a commit to biblepay/biblepay that referenced this pull request Nov 24, 2019
* commit '8b14adb206d76fbc6307385999a1c512052e93fa': (21 commits)
  [v0.14.0.x] Update release notes with change log (dashpay#3213)
  [v0.14.0.x] Bump version to 0.14.0.4 and draft release notes (dashpay#3203)
  Circumvent BIP69 sorting in fundrawtransaction.py test (dashpay#3100)
  Fix compile issues
  Merge bitcoin#11397: net: Improve and document SOCKS code
  Slightly optimize ApproximateBestSubset and its usage for PS txes (dashpay#3184)
  Update activemn if protx info changed (dashpay#3176)
  Actually update spent index on DisconnectBlock (dashpay#3167)
  Only track last seen time instead of first and last seen time (dashpay#3165)
  Merge bitcoin#17118: build: depends macOS: point --sysroot to SDK (dashpay#3161)
  Simulate BlockConnected/BlockDisconnected for PS caches
  Few fixes related to SelectCoinsGroupedByAddresses (dashpay#3144)
  Various fixes for mixing queues (dashpay#3138)
  Also consider txindex for transactions in AlreadyHave() (dashpay#3126)
  Ignore recent rejects filter for locked txes (dashpay#3124)
  Make orphan TX map limiting dependent on total TX size instead of TX count (dashpay#3121)
  Update/modernize macOS plist (dashpay#3074)
  Fix bip69 vs change position issue (dashpay#3063)
  Partially revert 3061 (dashpay#3150)
  Fix SelectCoinsMinConf to allow instant respends (dashpay#3061)
  ...

# Conflicts:
#	configure.ac
#	doc/man/biblepay-cli.1
#	doc/man/biblepay-qt.1
#	doc/man/biblepay-tx.1
#	doc/man/biblepayd.1
#	doc/release-notes.md
#	src/clientversion.h
#	src/wallet/wallet.cpp
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

Successfully merging this pull request may close these issues.

4 participants