Traffic Based App Rewards: Compute Hashes by adetokunbo · Pull Request #4847 · canton-network/splice

adetokunbo · 2026-04-06T12:31:32Z

Fixes #4382
Follows #4662
Tracked in #4384

Pull Request Checklist

Cluster Testing

If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If a logical synchronizer upgrade test is required (from canton-3.5), comment /lsu_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

Include any change that might be observable by our partners or affect their deployment in the release notes.
Specify fixed issues with Fixes #n, and mention issues worked on using #n
Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

dfordivam

Posting some comment, still doing the review of sqls

meiersi-da · 2026-04-07T06:56:57Z

+    * Uses a LEFT JOIN from activity totals to reward totals because parties
+    * whose reward fell below the threshold have no row in `app_reward_party_totals`.


This sounds off. It sounds like we are numbering the parties that had activity instead of numbering the ones that have rewards, could that be the case?

I might have accidentally caused that with a premature optimization to use party_seq_num for activity totals already.

Fixed: This has been addressed. Note that the schema changed here as suggested in other review comments

rautenrieth-da · 2026-04-07T08:55:30Z

+              r.total_app_reward_amount::text as amount,
+              (a.app_provider_party_seq_num / $batchSize) as batch_num
+            from #${Tables.appActivityPartyTotals} a
+            left join #${Tables.appRewardPartyTotals} r


From our discussion:

We don't need to include parties with 0 rewards.

Only use the reward party totals table. If possible, structure queries such that they only depend on the immediately preceding stage.

In this case, can we move app_provider_party_seq_num to the rewards table (only numbering the above-threshold parties), and thus avoid the join?

Thanks, I was looking around where we noted down this comment.😅

After carefully going through the schema now I understood the issue. We can't use LEFT JOIN with the guarantee of always getting the party, and using JOIN is a code smell (or worse). So in this case moving party to app_reward_party_totals, and having app_provider_party_seq_num in it is the better choice

This would also mean that seq_num_begin seq_num_end would be contiguous, ie end=begin+batch_size.

Fixed. See bc074a1 and e0160a2

rautenrieth-da · 2026-04-07T20:29:19Z

+        batch_type:
+          type: string
+          enum: [BatchOfBatches, BatchOfMintingAllowances]
+        child_hashes:


child_hashes and minting_allowances are mutually exclusive, have you considered using oneOf (it's used in a few other places in scan.yaml)?

Fixed: see cb37261

rautenrieth-da · 2026-04-08T00:13:09Z

+          from #${Tables.appRewardBatchHashes}
+          where history_id = $historyId and round_number = $roundNumber
+            and batch_level = (
+              select max(batch_level) from #${Tables.appRewardBatchHashes}


There is no index on the batch level. Is there some mechanism that keeps the table small, so that we can afford a full table scan? For example, are we planning on removing batches from the table once the rewards are issued?

Alternatively, you can have aggregateToRoot return the level of the root.

As there are two other places with batch_level query, it makes sense to add the index. (insertNextLevel and getChildBatchHashes)

Fixed, added in 1a7d985

rautenrieth-da · 2026-04-08T07:11:14Z

+        rootHash shouldBe defined
+        rootHash.value.rootHash.size shouldBe 32
+        latestRound.value shouldBe roundNumber


Would it make sense to include these checks in all other tests, instead of only checking the root hash in a single scenario?

Fixed: Added in 4ecefa2

rautenrieth-da · 2026-04-08T07:13:48Z

+      }
+    }
+
+    "computeRewardHashes — below-threshold party hashes same as explicit zero amount" in {


This test is not needed anymore if we exclude below-threshold parties in insertLeafBatches.

rautenrieth-da · 2026-04-08T07:21:13Z

-          )
-        )
+      action
+        .map(_ => logger.debug(s"Computed reward hashes for round $roundNumber."))


This will only log once every 10min, I think this can be an INFO level log. If you have readily available details (number of leaves, number of levels, ...), include them in the message. Such details are very useful for investigating issues during testing.

Fixed: 2cbb56c

rautenrieth-da · 2026-04-08T07:29:59Z

+            from #${Tables.appRewardBatchHashes}
+            where history_id = $historyId
+              and round_number = $roundNumber
+              and batch_hash = ${batchHash.toByteArray}


We're converting ByteString -> Array[Byte] here, and Array[Byte] -> ByteString when reading the hash in getResultAppRewardBatchHash. Ideally, we would only convert types when reading/writing HTTP requests/responses.

Would it make sense to create a named case class for the hash, where the value is stored as Array[Byte] (or whatever results in the least amount of conversions), and with methods to convert it to/from the HTTP representation?

Fixed: see 333702a

dfordivam · 2026-04-08T08:53:05Z

An issue I noticed; computeAndStoreRewards is the API exposed via ScanAppRewardsStore, but there are no unit test(s) for it. I know its not specifically related to this PR, but perhaps a test could be added inDbScanAppRewardsStoreTest now since all the parts are in-place.

adetokunbo · 2026-04-09T00:32:09Z

An issue I noticed; computeAndStoreRewards is the API exposed via ScanAppRewardsStore, but there are no unit test(s) for it. I know its not specifically related to this PR, but perhaps a test could be added inDbScanAppRewardsStoreTest now since all the parts are in-place

IIRC, this was discussed at the planning meeting for this feature branch.
My interpretation of the discussion was that it was decided to unit test all the parts of this function, but not the function itself and instead use an integration test, which is as you may have noted, is planned for the following PR

In such a unit test, one is really just checking the interaction between computeAndStoreRewards and the functions it calls that have been themselves unit-tested

either one attempts to cover all the cases that the called functions cover. which involves duplicating existing test cases and test data;
a subset with specific combinations of test data; maybe 1 case ?
or one uses a technique like mocking to directly confirm the interaction between computeAndStoreRewards and the function it calls; some call this is dubious technique; others claim that's its the only value such a test case provides. AFAICT is not used elsewhere in the splice codebase

@dfordivam: What do you have in mind, any of these or something else ?

dfordivam · 2026-04-09T02:29:53Z

What do you have in mind, any of these or something else ?

Well naturally "it depends on context". One scenario likely not covered by integration tests is the throw by assertCompleteActivity, another would be calling it on round already computed.
Another one is whether RewardComputationInputs is being properly used, but because this is so critical we will have to write an integration test for it. So unit test at this API level can be ignored.

adetokunbo · 2026-04-09T07:51:51Z

What do you have in mind, any of these or something else ?

Well naturally "it depends on context". One scenario likely not covered by integration tests is the throw by assertCompleteActivity, another would be calling it on round already computed. Another one is whether RewardComputationInputs is being properly used, but because this is so critical we will have to write an integration test for it. So unit test at this API level can be ignored.

Added these tests

dfordivam · 2026-04-09T09:32:02Z

+    *
+    * Used for rounds where no parties are above the reward threshold.
+    * Inserting this as a level-0 batch and root hash marks the round as
+    * computed, so that ComputeAppRewardsTrigger does not retry it, and


ComputeAppRewardsTrigger has been mentioned twice in this file, should be RewardComputationTrigger ?

adetokunbo · 2026-04-09T10:37:19Z

@rautenrieth-da, I think all the comments have been addressed, PTAL

rautenrieth-da

Thanks! The PR looks good to me now, the merge conflicts should be easy to resolve.

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

…g tables - numbers only parties that have rewards rather than activity - remove from app_activty_party_totals where it will no longer be needed - introduce it in app_reward_party_totals which only contains thresholded parties Signed-off-by: Tim Emiola <adetokunbo@emio.la>

- they now read only from app_reward_party_totals - Now that app_reward_party_totals carries app_provider_party and its own contiguous seq_nums, the LEFT JOIN back to app_activity_party_totals and the COALESCE fallback for below-threshold parties are unnecessary. - Updates various affect unittests - Improves the scaladoc comments to clarify what role the steps are playing Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

…ray[Byte] Signed-off-by: Tim Emiola <adetokunbo@emio.la>

…eRewards Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

… error Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Merges the feature branch for the scan computation part of CIP-104, consisting of the following individual PRs: * Add app activity computation store and schema (#4372) * Compute aggregateActivityTotals, add ComputeAppRewardTrigger, expose new endpoints (#4420) * Adds computeRewardTotals to DbScanAppRewardsStore (#4639) * Add DB versions of Splice.Amulet.CryptoHash (#4662) * Expand GetRewardAccountingActivityTotalsResponse (#4859) * Add counters to RewardComputationTrigger (#4860) * Compute Hashes (#4847) Signed-off-by: Tim Emiola <adetokunbo@emio.la> Signed-off-by: Timothy Emiola <adetokunbo@users.noreply.github.com> Co-authored-by: Simon Meier <simon@digitalasset.com> Co-authored-by: Robert Autenrieth <31539813+rautenrieth-da@users.noreply.github.com>

adetokunbo added this to the Traffic-Based App Rewards milestone Apr 6, 2026

adetokunbo requested review from dfordivam, meiersi-da and rautenrieth-da April 6, 2026 12:31

adetokunbo self-assigned this Apr 6, 2026

dfordivam reviewed Apr 7, 2026

View reviewed changes

adetokunbo mentioned this pull request Apr 7, 2026

Traffic-Based App Rewards: Add counters to RewardComputationTrigger #4860

Merged

7 tasks

meiersi-da reviewed Apr 7, 2026

View reviewed changes

dfordivam reviewed Apr 8, 2026

View reviewed changes

Comment thread ...an/src/test/scala/org/lfdecentralizedtrust/splice/scan/store/DbScanAppRewardsStoreTest.scala Outdated

rautenrieth-da reviewed Apr 8, 2026

View reviewed changes

adetokunbo force-pushed the adetokunbo/cip-104-add-compute-hashes-save-batched-hashes branch from d224320 to 1a7d985 Compare April 9, 2026 06:13

dfordivam reviewed Apr 9, 2026

View reviewed changes

Comment thread ...an/src/test/scala/org/lfdecentralizedtrust/splice/scan/store/DbScanAppRewardsStoreTest.scala Outdated

dfordivam reviewed Apr 9, 2026

View reviewed changes

adetokunbo requested review from meiersi-da and rautenrieth-da April 9, 2026 10:36

adetokunbo force-pushed the adetokunbo/cip-104-add-compute-hashes-save-batched-hashes branch from 9e1d0dd to c460acb Compare April 9, 2026 12:08

rautenrieth-da approved these changes Apr 9, 2026

View reviewed changes

adetokunbo added 8 commits April 13, 2026 00:00

[ci] Add a TODO to obtain activity_records_count from a DB column

5ba31d4

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Obtain activity_records_count without a JOIN

5fb3e62

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Add a function that computes the leaf hashes

74b38c4

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Add insertNextLevel and aggregateToRoot

56b6654

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Insert root hashes

0d4ead7

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Invoke the full computation from the trigger

e39694b

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Call hash_minting_allowance with the correct fallback for 0

e4348a0

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Add a new scan route that obtains the computed route hash

965f2a6

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added 17 commits April 13, 2026 00:00

Add lookupBatchByHash

715b89d

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Add batch reward accounting routes

f9fa692

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Add SQL explanations in the comments

cd0c341

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Handle empty rounds in computeRewardHashes

92861bf

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Add assertRootHash helper; use in computeRewardHashes tests

d31e220

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Use setupAndComputeHashes to further simplify computeRewardHashes tests

7e8b33c

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

Replace debug logging with more informative INFO logs

5d837df

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Add an index on batch that uses the batch level

0fef61c

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Introduce RewardHash to avoid unnecessary conversions to/from Ar…

19e0387

…ray[Byte] Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Extract the computeRewardHashes action and use it computeAndStor…

24cd2a3

…eRewards Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Extend the no-hashes unit test

1f93b95

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Adds store tests for computeAndStoreRewards

869a1f4

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Use 'oneOf' to define RewardAccountingBatchOfBatches subtypes

2a77342

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] s/ComputeAppRewardsTrigger/RewardComputationTrigger/

00e432d

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

[ci] Follow TransferCommandContractStatus to avoid typescript codegen…

4a93cdb

… error Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo force-pushed the adetokunbo/cip-104-add-compute-hashes-save-batched-hashes branch from c460acb to 4a93cdb Compare April 13, 2026 00:15

adetokunbo merged commit 49e4f60 into adetokunbo/feature-cip-104-scan-computation Apr 13, 2026
66 checks passed

adetokunbo deleted the adetokunbo/cip-104-add-compute-hashes-save-batched-hashes branch April 13, 2026 00:59

adetokunbo added a commit that referenced this pull request Apr 13, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

6b73154

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added a commit that referenced this pull request Apr 13, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

5e30abb

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added a commit that referenced this pull request Apr 13, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

369c24c

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added a commit that referenced this pull request Apr 14, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

0af52fc

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added a commit that referenced this pull request Apr 16, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

18444f2

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

adetokunbo added a commit that referenced this pull request Apr 17, 2026

Traffic Based App Rewards: Compute Hashes (#4847)

cfb0215

Signed-off-by: Tim Emiola <adetokunbo@emio.la>

		* Uses a LEFT JOIN from activity totals to reward totals because parties
		* whose reward fell below the threshold have no row in `app_reward_party_totals`.

Conversation

adetokunbo commented Apr 6, 2026

Pull Request Checklist

Cluster Testing

PR Guidelines

Merge Guidelines

Uh oh!

dfordivam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adetokunbo Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfordivam commented Apr 8, 2026

Uh oh!

adetokunbo commented Apr 9, 2026

Uh oh!

dfordivam commented Apr 9, 2026

Uh oh!

Uh oh!

adetokunbo commented Apr 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adetokunbo commented Apr 9, 2026

Uh oh!

rautenrieth-da left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

adetokunbo Apr 9, 2026 •

edited

Loading