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

More intelligent transaction requests #1749

Merged
merged 7 commits into from
Mar 6, 2020
Merged

Conversation

intricate
Copy link
Contributor

Closes #1161.

Implements the new plan.

@intricate intricate added networking consensus issues related to ouroboros-consensus byron Required for a Byron mainnet: replace the old core nodes with cardano-node labels Mar 5, 2020
@intricate intricate added this to the S8 2020-03-12 milestone Mar 5, 2020
@intricate intricate self-assigned this Mar 5, 2020
@intricate intricate requested review from mrBliss, dcoutts and nfrisby March 5, 2020 01:22
@intricate intricate marked this pull request as ready for review March 5, 2020 01:36
@intricate intricate force-pushed the intricate/1161-new-plan branch 2 times, most recently from 569abc0 to 8cd528b Compare March 5, 2020 01:56
@nfrisby
Copy link
Contributor

nfrisby commented Mar 5, 2020

I rebased my wip branch onto commit 8cd528b, and I no longer see the huge number of transactions. Great!

requestedTxIdsInFlight = requestedTxIdsInFlight st - reqNo,
unacknowledgedTxIds = unacknowledgedTxIds st
<> Seq.fromList (map fst txids),
availableTxids = availableTxids st
<> Map.fromList txids
}
mpSnapshot <- atomically mempoolGetSnapshot
serverIdle n (acknowledgeTxIdsInMempool st' mpSnapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure you and @dcoutts have discussed this already, but just checking again: is this the right and only place to look at the Mempool?

Copy link
Contributor Author

@intricate intricate Mar 5, 2020

Choose a reason for hiding this comment

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

In my discussions with @dcoutts , he noted that this seemed like the right place to do it, yeah. I also believe this is where it makes the most sense as well.

Would appreciate further confirmation from @dcoutts, though. 😃

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/API.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Mempool/API.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

LGTM

@intricate
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 6, 2020

@iohk-bors iohk-bors bot merged commit f4d1c4b into master Mar 6, 2020
@iohk-bors iohk-bors bot deleted the intricate/1161-new-plan branch March 6, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
byron Required for a Byron mainnet: replace the old core nodes with cardano-node consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More intelligent transaction requests
3 participants