Skip to content

Conversation

@sidhujag
Copy link
Member

fixes #391

  • miner code updated to avoid conflicting inputs related to asset allocations (on conflict is allowed per mempool rules if it is an asset allocation tx dbl spend attempt)
  • ZDAG status' updated to reflect for input conflict detection across all ancestors in mempool

Miners were not able to mine a block due to conflicting inputs caused by inadvertent test where input was relayed by nodes based on correct functioning of our protocol to relay dbl spends related to asset allocation.

To reproduce:

  1. Send regular sys or asset with input A
  2. Send asset with input A
  3. All nodes will propogate both transactions due to mempool policy
  4. Miner will have issue with mining because it checks for balance overflow on dbl spend attempts in mempool across asset transactions but is missing a check related to conflicting inputs related to asset transactions

Fix:

  1. Add mempool conflicting input detection prior to mining block along with the balance overflows checks currently in the mempool code for syscoin consensus.
  2. Update ZDAG protocol status to correctly detect for issues when inputs are conflicting, simply don't accept that transaction in point of sale, it is potentially not going to be mined.

willyko
willyko previously approved these changes Jan 31, 2020
Copy link

@willyko willyko left a comment

Choose a reason for hiding this comment

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

ACK

modify removeConflicts to return a bool (for miner) if conflict got removed so miner can try again to create a new block. Also miner verifies inputs are valid or rejects/clears tx because a remnant from a input conflict where one got mined and the other stays (removeConflicts returns false but the input isn't valid in the mainchain because its spent)

zdag uses existsConflict so it doesnt remove it and subsequent calls pass to verifyzdag in the event of an error the first time due to inputs conflict
willyko
willyko previously approved these changes Jan 31, 2020
txmempool has a comparator which does hash if the ancestor account is equal so fall back to time in that case
Copy link

@Keyare Keyare left a comment

Choose a reason for hiding this comment

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

Ack

return CTxMemPool::CompareIteratorByHash()(a, b);
// SYSCOIN
return a->GetTime() < b->GetTime();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a critical improvement by removing ordering by time in graph.cpp we simply modify the comparator to return time ordering if ancestor counts are equal, we want ancestor ordering to be higher prescedence anyways followed by time so this shoudl work for us.

@willyko willyko merged commit c3a0ad9 into master Feb 1, 2020
@sidhujag sidhujag deleted the dev-4.x-fix-inputs branch February 1, 2020 04:22
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
* ZDAG fixes + miner code related to input conflicts

fixes syscoin#391

* Update configure.ac

* add existsConflict logic

modify removeConflicts to return a bool (for miner) if conflict got removed so miner can try again to create a new block. Also miner verifies inputs are valid or rejects/clears tx because a remnant from a input conflict where one got mined and the other stays (removeConflicts returns false but the input isn't valid in the mainchain because its spent)

zdag uses existsConflict so it doesnt remove it and subsequent calls pass to verifyzdag in the event of an error the first time due to inputs conflict

* remove graph ordering by time logic

txmempool has a comparator which does hash if the ancestor account is equal so fall back to time in that case

* rm graph

Co-authored-by: Jonas Schnelli <[email protected]>
sidhujag pushed a commit that referenced this pull request Aug 5, 2021
d54d949 qt: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov)

Pull request description:

  Fix #392.

  Adding a new item to the `m_wallet_selector` must follow the establishment of a connection between the `WalletView::encryptionStatusChanged` signal and the `BitcoinGUI::updateWalletStatus` slot.

  This was a regression introduced in bitcoin@20e2e24 (#29).

  ---

  An _encrypted_ wallet being auto-loaded at the GUI startup:
  - on master (eaf09bd)

  ![Screenshot from 2021-08-03 22-38-49](https://user-images.githubusercontent.com/32963518/128075837-cdbb2047-5327-43ea-b2d5-2dcdef67cdc0.png)

  - with this PR

  ![Screenshot from 2021-08-03 22-34-58](https://user-images.githubusercontent.com/32963518/128075572-cb727652-ad44-4b85-bf64-edcd19f9dea1.png)

ACKs for top commit:
  achow101:
    ACK d54d949
  jarolrod:
    ACK d54d949

Tree-SHA512: 669615ec8e1517c2f4cdf59bd11a7c85be793ba0dda112361cf95e6c2f0636215fed331d26a86dc9b779a49defae1b248232f98dab449584376c111c288e87bb
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.

Input conflict causing miners to not be able to miner

5 participants