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

Add an option to avoid schedule receipt check immediately #124

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

Chengxuan
Copy link
Contributor

@Chengxuan Chengxuan commented Jun 11, 2024

Currently, when a new transaction is registered for receipt, the confirmation manager schedules the check immediately. This caused two problems:

  • If the transactions are new, the first receipt check is almost guaranteed to be wasted as the transaction wouldn't have been mined into a block yet.
  • if the receipt was retrieve, duplicate receipt checks are possible due to a racing condition with the logic that schedules transaction receipt in processBlock. 8e15e84 demonstrates a naive fix from a different angle, but it won't work with forking situation.

The proposed PR delay the first fetch to when a new block is received or when the transaction has queued for longer than stale timeout. The behavior is put under a new configuration.

Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
@Chengxuan Chengxuan requested a review from a team as a code owner June 11, 2024 16:31
@Chengxuan Chengxuan changed the title Add an option to schedule receipt check in a delayed manner. Add an option to avoid schedule receipt check immediately Jun 11, 2024
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

@Chengxuan - question before approval here... what about the case where the block has already been mined by the time we add the transaction to the list.

This is concretely possible I think.

One example would be unlucky timing (particularly on a mine-on-demand chain).
Another would be if we've been down for a good period of time and are recovering transactions from the DB.

This I believe was the original reason for having the immediate check.

I don't disagree with removing it, but I wonder if the stale timeout is actually much too long to wait for a block to arrive.

config.md Outdated
@@ -44,6 +44,7 @@
|Key|Description|Type|Default Value|
|---|-----------|----|-------------|
|blockQueueLength|Internal queue length for notifying the confirmations manager of new blocks|`int`|`50`
|fetchReceiptUponEntry|Fetch receipt of new transactions immediately when they are added to the internal queue. When set to false, fetch will only happen when a new block is received or the transaction has been queue for more than the stale receipt timeout|`boolean`|`true`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default of false here would fine if it seems like the current behavior is undesirable in all use cases we're aware of.

We have some other big changes queued up for FFTM, so I feel like we should consider adding some notes to the next release. Maybe ensuring that there's a rollup of some of those notes to FireFly 1.3.1 when it's released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, will make the change

@Chengxuan
Copy link
Contributor Author

Chengxuan commented Jun 11, 2024

@peterbroadhurst in that case, the unscheduled transactions will be scheduled to get receipt whenever one of the following event happens:

  1. the first time when any new block is detected (all unscheduled txs will get scheduled at this point to avoid missing historical txs that won't be mined in new blocks ever).
  2. transaction has been in the queue for longer than staleReceiptTimeout
  3. the block that contains the unscheduled transition is detected

In other words, for a subset of transactions (should be relatively small unless lots of transactions are not stuck in the tx pool on the node) "checking too early" situation. I don't think the "checking too early" situation can be avoided fully due to the nature of the problem.

Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Thanks for the extra detail.

Give more to think about in terms of efficiency, but makes it clear this PR is great step forwards.

@peterbroadhurst peterbroadhurst merged commit 32d8a0c into hyperledger:main Jun 12, 2024
2 checks passed
@peterbroadhurst peterbroadhurst deleted the receipt-schedule-change branch June 12, 2024 13:53
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.

2 participants