Use cache in `BlockchainController.GetTransactionsAsync` (take 2) #12275

kiminuo · 2024-01-18T09:23:17Z

Alternative to #12263

WalletWasabi.Backend/Controllers/BlockchainController.cs

WalletWasabi/Cache/IdempotencyRequestCache.cs

kiminuo · 2024-01-18T09:28:00Z

WalletWasabi.Backend/Controllers/BlockchainController.cs

+			uint256 txId = parsedTxIds[i];
+			string cacheKey = GetCacheKeyForTransaction(txId);
+
+			if (Cache.TryGetValue(cacheKey, out TaskCompletionSource<Transaction>? tcs))


Maybe it would be better to be more defensive and verify that tcs.Task is not a failed task which can happen I think.

kiminuo · 2024-01-18T09:28:45Z

WalletWasabi.Backend/Controllers/BlockchainController.cs

@@ -140,76 +143,107 @@ private async Task<IEnumerable<string>> GetRawMempoolStringsNoCacheAsync(Cancell
 	[HttpGet("transaction-hexes")]
 	[ProducesResponseType(200)]
 	[ProducesResponseType(400)]
-	public async Task<IActionResult> GetTransactionsAsync([FromQuery, Required] IEnumerable<string> transactionIds, CancellationToken cancellationToken)
+	public async Task<IActionResult> GetTransactionsAsync([FromQuery, Required] List<string> transactionIds, CancellationToken cancellationToken)


I think this is safe but maybe not. Not sure. If not, we can create a copy of the input parameter.

I would revert this.

If you want to change it setup a RegTest and make sure it is working when this happens:

https://github.com/molnard/WalletWasabi/blob/master/WalletWasabi/CoinJoin/Client/CoinJoinProcessor.cs#L53

Or was that already done that's why it was resolved?

WalletWasabi.Backend/Controllers/BlockchainController.cs

WalletWasabi/Cache/IdempotencyRequestCache.cs

kiminuo · 2024-01-18T12:13:28Z

More or less ready. We should have unit tests. Anyway, this PR waits for concept ACK / NACK.

turbolay · 2024-03-08T13:13:33Z

@molnard Can you take a decision about this PR or the other alternate and work towards merge please?

adamPetho

I'm getting 500 Internal Server Error no matter what. I debugged into the backend and the transactions are fetched properly, but there is a problem with serialization.

fail: Microsoft.AspNetCore.Server.Kestrel[13]
      Connection id "0HN22KEDCI5QI", Request id "0HN22KEDCI5QI:00000005": An unhandled exception was thrown by the application.
      Newtonsoft.Json.JsonSerializationException: Error getting value from 'Date' on 'NBitcoin.LockTime'.
       ---> System.InvalidOperationException: This is not a time based lock
         at lambda_method167(Closure, Object)
         at Newtonsoft.Json.Serialization.ExpressionValueProvider.GetValue(Object target)
         --- End of inner exception stack trace ---
         at Newtonsoft.Json.Serialization.ExpressionValueProvider.GetValue(Object target)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.CalculatePropertyValues(JsonWriter writer, Object value, JsonContainerContract contract, JsonProperty member, JsonProperty property, JsonContract& memberContract, Object& memberValue)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeValue(JsonWriter writer, Object value, JsonContract valueContract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerProperty)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty)
         at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeValue(JsonWriter writer, Object value, JsonContract valueContract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerProperty)

WalletWasabi.Backend/Controllers/BlockchainController.cs

molnard · 2024-03-12T15:41:18Z

@molnard Can you take a decision about this PR or the other alternate and work towards merge please?

It's fine we will make the decision with @adamPetho when it will be required.

adamPetho · 2024-03-12T16:21:55Z

It's fine we will make the decision with @adamPetho when it will be required.

It is required right now, so it's time to make up our minds.

Personally, I don't care which approach gets merged in as long as it's working and does the job.

According to Kimi #12263 (review) that PR has major bugs.

According to my testing, this PR is almost working, we only need to fix the return type.

Whichever PR we decide on, it will be a great help on my current task. Just please choose and push one of these forward.

Co-authored-by: adamPetho <45069029+adamPetho@users.noreply.github.com>

molnard

cACK

Please prepare the method like this for adamp. We will merge this PR first.

https://github.com/zkSNACKs/WalletWasabi/pull/12302/files#diff-67d44169c752110836b10bbc845870d2ebc2f3bd4246136f16365fa26058918cR508

molnard

Can you make this happen? A running version that is tested and not PoC?

WalletWasabi.Backend/Controllers/BlockchainController.cs

adamPetho

tACK, Code LGTM

WalletWasabi.Backend/Controllers/BlockchainController.cs

molnard

LGTM, couple of small things to consider but in general OK.

molnard · 2024-03-15T13:50:04Z

WalletWasabi.Backend/Controllers/BlockchainController.cs

 		}
 		catch
 		{
 			return BadRequest("Invalid transaction Ids.");
 		}

+		Transaction[] txs = await FetchTransactionsAsync(parsedTxIds, cancellationToken).ConfigureAwait(false);


Was that intentional to go with the Internal Server Error in case of exception instead of BadRequest(ex.Message); like it was before? If so what is the benefit and how will the client react to that?

Reverted back in 70a9b75

molnard · 2024-03-15T13:58:42Z

WalletWasabi.Backend/Controllers/BlockchainController.cs

@@ -32,6 +36,7 @@ public class BlockchainController : ControllerBase
 {
 	public static readonly TimeSpan FilterTimeout = TimeSpan.FromMinutes(20);
 	private static readonly MemoryCacheEntryOptions CacheEntryOptions = new() { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(60) };
+	private static MemoryCacheEntryOptions TransactionCacheOptions { get; } = new MemoryCacheEntryOptions { AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(1) };


How long should we store the tx in the cache? Can it change meanwhile?

Transactions are not changing. Mempool information is stored on another level but the tx will remain the same. So we could store the tx indefinitely - nothing gets outdated.

On the other hand, we need to preserve the memory, so the must be a timeout when after we release the memory.

Let's do a practical approach, the client uses this request the most while transition are in the mempool mostly when it appeared in the mempool with the sync requests. The request interval is 30 seconds. However the new functionality will add one more use case with the unconfirmed tx chain that is requesting may transactions from time to time.
Are there any risks if we increase this to like to 20 minutes?

Changed to 20 minutes.

molnard · 2024-03-15T14:01:24Z

WalletWasabi.Backend/Controllers/BlockchainController.cs

@@ -140,76 +143,107 @@ private async Task<IEnumerable<string>> GetRawMempoolStringsNoCacheAsync(Cancell
 	[HttpGet("transaction-hexes")]
 	[ProducesResponseType(200)]
 	[ProducesResponseType(400)]
-	public async Task<IActionResult> GetTransactionsAsync([FromQuery, Required] IEnumerable<string> transactionIds, CancellationToken cancellationToken)
+	public async Task<IActionResult> GetTransactionsAsync([FromQuery, Required] List<string> transactionIds, CancellationToken cancellationToken)


I would revert this.

If you want to change it setup a RegTest and make sure it is working when this happens:

https://github.com/molnard/WalletWasabi/blob/master/WalletWasabi/CoinJoin/Client/CoinJoinProcessor.cs#L53

Or was that already done that's why it was resolved?

molnard

LGTM.

AbsoluteExpirationRelativeToNow I am not sure about. WDY guys think about such a long caching?

#12275 (comment)

kiminuo · 2024-03-15T18:20:02Z

Storing a transaction in memory for some time is cheaper than repeatedly getting it via RPC.

turbolay

It is hard for me to review this PR but LGTM, I've seen no issues in it.

WalletWasabi.Backend/Controllers/BlockchainController.cs

turbolay · 2024-03-16T01:59:23Z

WalletWasabi/Cache/IdempotencyRequestCache.cs

+	/// </summary>
+	/// <returns><c>true</c> if the key was added to the cache, <c>false</c> otherwise.</returns>
+	/// <remarks>Caller is responsible to ALWAYS set a result to <paramref name="responseTcs"/> even if an exception is thrown.</remarks>
+	public bool TryAddKey<TRequest, TResponse>(TRequest cacheKey, MemoryCacheEntryOptions options, out TaskCompletionSource<TResponse> responseTcs)


This is a good function, it might be useful elsewhere

Yeah, though one must be extra cautious when using it.

I think I know how one can make it safer but it would be more lines of code, so ... :)

Well the original PR created by me was not touching IdempotenceRequestCache at all. But that was down-voted so this is the solution you guys wanted.

I'm not aware of any downvoting per se (only major bugs in your PR that were not addressed).

#12263 (comment)

It doesn't make sense to address any bugs if the concept is not ACKed by the godfather of IdempotencyRequestCache.

You guys went with the alternative solution which is perfectly fine for me. Now that we are at the end of this, are you saying that adding TryAddKey is not safe? I hope this is not the case...

It's not safe in the sense that if one forgets to assign the result, then bad things will happen. This is not an issue if one has good tests.

Personally, I would like more to return from TryAddKey a disposable that would make sure that either a result is assigned or an exception is assigned and if not, it would throw an exception so it would be very much visible that something unexpected has happened. Anyway, even without this it should work well but one must be careful.

molnard

ACK - feel free to merge @kiminuo if it is ready.

molnard · 2024-03-18T09:50:46Z

WalletWasabi/Cache/IdempotencyRequestCache.cs

+	/// </summary>
+	/// <returns><c>true</c> if the key was added to the cache, <c>false</c> otherwise.</returns>
+	/// <remarks>Caller is responsible to ALWAYS set a result to <paramref name="responseTcs"/> even if an exception is thrown.</remarks>
+	public bool TryAddKey<TRequest, TResponse>(TRequest cacheKey, MemoryCacheEntryOptions options, out TaskCompletionSource<TResponse> responseTcs)


Well the original PR created by me was not touching IdempotenceRequestCache at all. But that was down-voted so this is the solution you guys wanted.

molnard · 2024-03-18T09:55:13Z

CF issues are not related to this PR - but discovered by CF now. Don't need to fix them here.

kiminuo · 2024-03-18T10:55:21Z

ACK - feel free to merge @kiminuo if it is ready.

I cannot:

molnard · 2024-03-18T11:21:30Z

Oh Got it, because of CF... Can I merge then?

kiminuo · 2024-03-18T11:27:55Z

Oh Got it, because of CF...

I think so.

Can I merge then?

Yes.

molnard and others added 3 commits January 16, 2024 16:25

Use cache at API GetTransactionsAsync - parallel

aa1bd7c

Remove old cache

f2e1580

WIP

f7ebde5

pull-request-size bot added the size/L label Jan 18, 2024

kiminuo mentioned this pull request Jan 18, 2024

Use cache at API GetTransactionsAsync - parallel #12263

Closed

kiminuo commented Jan 18, 2024

View reviewed changes

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

kiminuo commented Jan 18, 2024

View reviewed changes

WalletWasabi/Cache/IdempotencyRequestCache.cs Outdated Show resolved Hide resolved

kiminuo commented Jan 18, 2024

View reviewed changes

molnard requested changes Jan 18, 2024

View reviewed changes

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

WalletWasabi/Cache/IdempotencyRequestCache.cs Outdated Show resolved Hide resolved

kiminuo added 11 commits January 18, 2024 12:16

Merge branch 'master' into feature/2024-01-18-gettxcache

6d2b140

Feedback

f28f11f

comment

4de8bef

more

2b0a15d

Simplify

7ae8a83

simplify

bc026a9

scope

3250e0f

simplify

f24a3af

Improve

abec20c

Not empty

e60c06e

remove special case for n=1

e92531c

adamPetho requested changes Mar 12, 2024

View reviewed changes

adamPetho reviewed Mar 12, 2024

View reviewed changes

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

kiminuo and others added 2 commits March 12, 2024 18:33

Update WalletWasabi.Backend/Controllers/BlockchainController.cs

553a619

Co-authored-by: adamPetho <45069029+adamPetho@users.noreply.github.com>

refactor

012fe2d

molnard assigned kiminuo Mar 13, 2024

molnard requested changes Mar 13, 2024

View reviewed changes

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

kiminuo added 2 commits March 14, 2024 09:22

fb

b1c5383

Add a test

6263538

kiminuo force-pushed the feature/2024-01-18-gettxcache branch from b128a68 to 6263538 Compare March 14, 2024 08:22

Update implementation

8a7012b

kiminuo marked this pull request as ready for review March 14, 2024 09:51

kiminuo requested review from adamPetho and turbolay March 14, 2024 11:24

Extract a method

922ad90

adamPetho previously approved these changes Mar 14, 2024

View reviewed changes

WalletWasabi.Backend/Controllers/BlockchainController.cs Outdated Show resolved Hide resolved

remove unused variable

a3f2b7f

kiminuo dismissed adamPetho’s stale review via a3f2b7f March 14, 2024 15:05

Merge branch 'master' into feature/2024-01-18-gettxcache

9975500

adamPetho requested a review from molnard March 14, 2024 16:04

molnard requested changes Mar 15, 2024

View reviewed changes

fb

70a9b75

molnard reviewed Mar 15, 2024

View reviewed changes

turbolay approved these changes Mar 16, 2024

View reviewed changes

molnard approved these changes Mar 18, 2024

View reviewed changes

molnard merged commit 8fa5f97 into zkSNACKs:master Mar 18, 2024
7 of 8 checks passed

kiminuo deleted the feature/2024-01-18-gettxcache branch March 18, 2024 12:09

turbolay mentioned this pull request May 16, 2024

Outputs fails to register because AlreadyRegisteredScript #12856

Closed

Use cache in BlockchainController.GetTransactionsAsync (take 2) #12275

Use cache in BlockchainController.GetTransactionsAsync (take 2) #12275

Conversation

kiminuo commented Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiminuo commented Jan 18, 2024

turbolay commented Mar 8, 2024

adamPetho left a comment

Choose a reason for hiding this comment

molnard commented Mar 12, 2024

adamPetho commented Mar 12, 2024

molnard left a comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

adamPetho left a comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

kiminuo commented Mar 15, 2024 • edited

turbolay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard commented Mar 18, 2024

kiminuo commented Mar 18, 2024

molnard commented Mar 18, 2024 • edited

kiminuo commented Mar 18, 2024

Use cache in `BlockchainController.GetTransactionsAsync` (take 2) #12275

Use cache in `BlockchainController.GetTransactionsAsync` (take 2) #12275

kiminuo commented Jan 18, 2024 •

edited

kiminuo commented Mar 15, 2024 •

edited

molnard Mar 18, 2024 •

edited

molnard commented Mar 18, 2024 •

edited