Validate checkpoint

[binary-ho]

1. What this PR does / why we need it:

This issue is validation of the Checkpoint when a client calls the PushPull API and sends a request ChangePack. This validation is necessary to handle potential issues such as network-induced duplicate requests, bugs in the new SDK, or malicious tampering.

Specifically, three scenarios needed to be validated:

1. The ClientSeqs in the reqPacks.Changes are sequential.
2. The ClientSeqs in the request is sequential with DocInfo.Checkpoint.ClientSeq.
3. The ClientSeq in the request sent to the cannot be less than the ClientSeq in the Server's ClientInfo. This addresses duplicate requests.
4. The ServerSeq in the request sent to the PushPull API cannot be greater than the ServerSeq maintained by the server. This addresses malicious requests.

I will refer to these three scenarios as Case 1, 2, 3, 4

Upon reviewing the code, I found that Cases 3 and 4 were already implemented.

Thus, I only needed to implement Case 1. However, I have written test codes for Cases 1, 2, 3, 4 and the happy case. I will explain the implementation for each case and the corresponding test code.

In the upcoming code blocks, the first line of each comment will indicate the file name and function name for your reference.

I also have a few questions. These questions are marked in bold and will be included in the Special notes for your reviewer section.

1.1 Case 1: The ClientSeqs in the request sent to the PushPull API are sequential

This validation is implemented in packs.go and occurs immediately when the PushPull function is called.

server/packs/packs.go

func validateClientSeqSequential(changes []*change.Change) error {
	if len(changes) <= 1 {
		return nil
	}

	nextClientSeq := changes[0].ClientSeq()
	for _, cn := range changes[1:] {
		nextClientSeq++

		if nextClientSeq != cn.ClientSeq() {
			return fmt.Errorf(
				"ClientSeq in Changes are not sequential (expected: %d, actual: %d) : %w",
				nextClientSeq,
				cn.ClientSeq(),
				ErrClientSeqNotSequential,
			)
		}
	}
	return nil
}

This function checks if the Changes in the request are sequential.

Q. It does not validate whether the first Change's ClientSeq differs by 1 from the ClientSeq in the existing ClientInfo. Should this be added? Logically, if there are 0 or 1 changes, there's nothing to validate, so it returns without issues. If the sequence is not sequential, it returns ErrClientSeqNotSequential, which is mapped to connect.CodeInvalidArgument in the response via errorToConnectCode.

server/packs/packs.go

func PushPull(
    ...
	// TODO: Changes may be reordered or missing during communication on the network.
	// We should check the change.pack with checkpoint to make sure the changes are in the correct order.
	err := validateClientSeqSequential(reqPack.Changes)
	if err != nil {
		return nil, err
	}
    ...

This validation is tested at the location of the above comment.

Q. Should we now remove this TODO comment?

1.2 `Case 2: The ClientSeq in changes not sequential with DocInfo.Checkpoint.ClientSeq

// packs/packs.go
func validateClientSeqSequentialWithCheckpoint(changes []*change.Change, checkpoint change.Checkpoint) error {
	expectedClientSeq := checkpoint.ClientSeq + 1
	actualFirstClientSeq := changes[0].ClientSeq()

	if expectedClientSeq < actualFirstClientSeq {
		return fmt.Errorf(
			"ClientSeq is not sequential with DocInfo.Checkpoint.ClientSeq (expected: %d, actual: %d) : %w",
			expectedClientSeq,
			actualFirstClientSeq,
			ErrClientSeqNotSequentialWithCheckpoint,
		)
	}
	return nil
}

these two case return connect.CodeInvalidArgument

// server\rpc\connecthelper\status.go

var errorToConnectCode = map[error]connect.Code{
	
       ...

	packs.ErrClientSeqNotSequentialWithCheckpoint: connect.CodeInvalidArgument,
	packs.ErrClientSeqInChangesAreNotSequential:   connect.CodeInvalidArgument,

}

...

1.3 Case 3: The ClientSeq in changes cannot be less than the ClientSeq in the Server's ClientInfo

This was already implemented, and it simply logs a warning. It returns OK as specified in the current issue.

pushpull.go, function pushChanges()

    ...
	cp := clientInfo.Checkpoint(docInfo.ID)

	var pushedChanges []*change.Change
	for _, cn := range reqPack.Changes {
		if cn.ID().ClientSeq() > cp.ClientSeq {
			serverSeq := docInfo.IncreaseServerSeq()
			cp = cp.NextServerSeq(serverSeq)
			cn.SetServerSeq(serverSeq)
			pushedChanges = append(pushedChanges, cn)
		} else {
			logging.From(ctx).Warnf(
				"change already pushed, clientSeq: %d, cp: %d",
				cn.ID().ClientSeq(),
				cp.ClientSeq,
			)
		}

		cp = cp.SyncClientSeq(cn.ClientSeq())
	}
    ...

1.4 Case 4: The ServerSeq in the request cannot be greater than the ServerSeq maintained by the server.

This was also already implemented. In the code below, initialServerSeq refers to the ServerSeq maintained in the Document Info.

Q. Currently, it returns connect.CodeFailedPrecondition instead of connect.CodeInvalidArgument as specified in the issue. Is this appropriate?

pushpull.go pullPack

    ...
    if initialServerSeq < reqPack.Checkpoint.ServerSeq {
		return nil, fmt.Errorf(
			"serverSeq of CP greater than serverSeq of clientInfo(clientInfo %d, cp %d): %w",
			initialServerSeq,
			reqPack.Checkpoint.ServerSeq,
			ErrInvalidServerSeq,
		)
	}
    ...

1.5 Test Code

The test code is located in server/packs/packs_test.go. It covers the following four scenarios:

// server/packs/packs_test.go

func Test(t *testing.T) {
	t.Run("push/pull sequential ClientSeq test (happy case)", func(t *testing.T) {
		RunPushPullWithSequentialClientSeqTest(t)
	})

	t.Run("push/pull not sequential ClientSeq test", func(t *testing.T) {
		RunPushPullWithNotSequentialClientSeqTest(t)
	})

	t.Run("push/pull ClientSeq less than ClientInfo's ClientSeq (duplicated request)", func(t *testing.T) {
		RunPushPullWithClientSeqLessThanClientInfoTest(t)
	})

	t.Run("push/pull ServerSeq greater than DocInfo's ServerSeq", func(t *Testing.T) {
		RunPushPullWithServerSeqGreaterThanDocInfoTest(t)
	})
}

1. happy case: Sequential ClientSeq in Client Request
2. Not Sequential with DocInfo.Checkpoint.ClientSeq
3. Not Sequential ClientSeq in Client Request Changes
4. duplicated request: ClientSeq in Request is less than ClientInfo's ClientSeq (But No Error)
5. ServerSeq in Request is greater than DocInfo's ServerSeq

2. Which issue(s) this PR fixes:

3. Special notes for your reviewer:

Questions 1 through 3 are addressed in the case descriptions above:

1. Case 1: The function validateClientSeqSequential() verifies that the Changes in the request are sequential. It does not validate whether the first Change's ClientSeq differs by 1 from the ClientSeq in the existing ClientInfo. Should this be added?

2. Case 1: Should we remove the TODO comment in the PushPull function in server/packs/packs.go?

3. Case 3: The issue specifies that malicious ServerSeqs should return connect.CodeInvalidArgument, but currently, it returns connect.CodeFailedPrecondition. Is this appropriate?

4. If client actions are required in response to these scenarios, we should discuss how to handle them.

   • e.g., The client should be aware that Changes are not applied if the request is not sequential.

4. Does this PR introduce a user-facing change?:

5. Additional documentation:

Here, I have attached the questions I posted on the issue and the answers I found. I hope this will be helpful to those studying ClientSeq and ServerSeq. If there are any mistakes, please let me know.

I have also attached the responses from @sejongk to my questions. I learned a lot from his answers, and I am very grateful to him.

5.1 My Question And Answer

Hello,
I have a few questions as I work on understanding the content to resolve the issue.

• Issue: Validate Checkpoint in ChangePack for PushPull API requests

Q1. Where do the server's ClientSeq and ServerSeq come from?

To resolve this issue, I thought it would be sufficient to compare the ClientSeq and ServerSeq of the client with those maintained by the server.

Is it correct that the server's ClientSeq and ServerSeq can be obtained from the Checkpoint of ClientInfo, which is derived from the ClientId in the Request Message?

Q2. Where do the ClientSeq and ServerSeq sent by the client come from?

The ChangePack in the Request's Msg contains a Checkpoint and an array of Changes.
The Checkpoint holds its ClientSeq and ServerSeq, and each Change in the array also has its ClientSeq and ServerSeq.
According to the issue, it mentions "so Change.ID.Checkpoint.ClientSeq should increment sequentially by one." Does this mean that the ClientSeq of the client should be the sequential values of each Change, and these should be validated against the server's ClientSeq (rather than the ClientSeq in the Checkpoint of the ChangePack)?

And regarding "Checkpoint.ServerSeq in the request ChangePack for PushPull API cannot be greater than the server's Checkpoint.ServerSeq since it is set when the server saves the Change to the database."

Should the client's ServerSeq be obtained from the ServerSeq in the Checkpoint of the ChangePack in the Msg (not the ServerSeq of each individual Change)?

Answer 1, 2

1. The server's ServerSeq can be found in DocInfo. Since the ServerSeq is updated every time a Change from various clients is applied, it is managed at the Document level.
2. The server's ClientSeq can be found in the Checkpoint of ClientInfo. Since the server needs to know the last ClientSeq sent by the client, it is managed at the ClientInfo level.
3. The client's ClientSeq is assigned by the client for each Change and is sequentially included in Changes. The ClientSeq can be found in both the Checkpoint and the Changes.
4. The client's ServerSeq can be found in reqPack.Checkpoint.ServerSeq. The client retains the last ServerSeq received during synchronization and includes it in the Checkpoint when sending a Request.

Q3. How is a "wrong" ClientSeq determined?

You mentioned that Change.ID.Checkpoint.ClientSeq should increment sequentially by one. Does this mean that the following scenarios are considered wrong?

1. If the ClientSeq of each Change in the ChangePack sent by the client is not sequential, the request is wrong.
2. If the ClientSeq in the CheckPoint sent by the client is ≤ the _ClientSeq in clientInfo, it is wrong.

Answer 3

1. Correct. There was an issue during the process of saving the Changes.
2. Correct. This corresponds to a duplicate request.

Q4. How is a duplicate request determined?

Would it be correct to consider the scenario where the ClientSeq sent by the client is equal to the _ClientSeq in clientInfo as a duplicate request due to network delay? I seem to be missing the criteria for determining a "duplicate request."

Answer 4

Answered in Answer 3.

Q5. How is a wrong ServerSeq determined?

Is it wrong when the ServerSeq sent by the client is greater than the ServerSeq obtained from ClientInfo on the server?

Answer 5

If the ServerSeq sent by the client is greater than the ServerSeq found in DocInfo on the server, it is a malicious request. This is because the ServerSeq held by the client is initially provided by the server. Since the ServerSeq increases sequentially, the client's ServerSeq should never be larger.

Q6. Where should the validation take place?

I believe it should occur in the PushPullChanges function in yorkie_server.go. Should I declare the validate function and error directly in yorkie_server.go, or should I create a validator file in the rpc package and declare the validate function and error there?

Answer 6

I was curious whether a separate object or layer was needed for validation. Based on Yorkie's existing validation methods, it seems unnecessary.

5.2 sejongk's Answer

Understanding the concept of a Checkpoint seems crucial for this issue. I searched for relevant design documents and found a document on GC garbage-collection.md , though it might be good to add more detailed information about Checkpoints there or even create a separate Checkpoint document in the future.

You may already be familiar with Checkpoints, but I'll share what I reviewed today:

• The Yorkie server imposes sequential consistency on the Changes generated by multiple clients. In other words, the server assigns the id of the Changes received during the push process with a global counter called server_seq, which is incremented by 1. The docInfo.SeverSeq corresponds to this global counter.
• Later, during the pull process, the server sends the Changes that the client has not yet received, along with the current server_seq that has been incremented up to that point. This server_seq serves to inform the client of how far it has received the Changes.
• When the client performs a pushpull, it sends the server_seq it previously received to the server in reqPack.Checkpoint.ServerSeq, notifying the server of how far it has received and prompting the server to send the Changes corresponding to the subsequent server_seq.
• The server holds the client_seq on the client side as clientInfo.Checkpoint(docInfo.ID).ClientSeq, which is used to validate the client_seq of the Changes.
  • ChangePack is used as a group for buffering Changes. The client_seq within the ChangePack must be sorted and incremented by 1 sequentially. This is because if a Change is lost due to network issues, it can be detected through the client_seq. This is similar to how sequence numbers are assigned to each packet in TCP.
  • It also solves the problem of duplicate Changes, as the server currently checks whether the client_seq is smaller than the client_seq the server holds (if cn.ID().ClientSeq() > cp.ClientSeq).
• The Checkpoint logic is crucial in GC, but since it's not the main point of this issue, I left it out. min_synced_seq is one of the core elements in GC.

After reviewing the above, it would be helpful to look at the diagrams explaining the Checkpoint flow at the following links:

• https://miro.com/app/board/o9J_kidjCtk=/?moveToWidget=3458764571662877621&cot=14
• https://miro.com/app/board/o9J_kidjCtk=/?moveToWidget=3458764554509936673&cot=14

You may want to review the code in these files:

• server/packs/packs.go/func PushPull
• server/packs/pushpull.go/func pushChanges
• server/packs/pushpull.go/func pullChanges

6. Checklist:

• validate ClientSeqs in the request sent to the PushPull API are sequential.
• validate ClientSeq in the request sent to the PushPull API cannot be less than the ClientSeq in the Server's ClientInfo. This means duplicate requests.
• validate ServerSeq in the request sent to the PushPull API cannot be greater than the ServerSeq maintained by the server. This means malicious requests.
• Write test code

Summary by CodeRabbit

• New Features

  • Enhanced error handling for client sequence validation during document synchronization.
  • Introduced new error types for non-sequential client sequences.

• Bug Fixes

  • Improved control flow in document processing to prevent invalid changes from being processed.
  • Added specific error mappings to enhance feedback during connection issues related to client sequence validation.

• Tests

  • Added comprehensive unit tests for validating client sequence handling in push and pull operations.

[github-actions]

Go Benchmark

Benchmark suite	Current: 0217951	Previous: 6d13dc9	Ratio
BenchmarkDocument/constructor_test	1493 ns/op 1337 B/op 24 allocs/op	1499 ns/op 1337 B/op 24 allocs/op	1.00
BenchmarkDocument/constructor_test - ns/op	1493 ns/op	1499 ns/op	1.00
BenchmarkDocument/constructor_test - B/op	1337 B/op	1337 B/op	1
BenchmarkDocument/constructor_test - allocs/op	24 allocs/op	24 allocs/op	1
BenchmarkDocument/status_test	1121 ns/op 1305 B/op 22 allocs/op	1105 ns/op 1305 B/op 22 allocs/op	1.01
BenchmarkDocument/status_test - ns/op	1121 ns/op	1105 ns/op	1.01
BenchmarkDocument/status_test - B/op	1305 B/op	1305 B/op	1
BenchmarkDocument/status_test - allocs/op	22 allocs/op	22 allocs/op	1
BenchmarkDocument/equals_test	9107 ns/op 7273 B/op 132 allocs/op	7711 ns/op 7273 B/op 132 allocs/op	1.18
BenchmarkDocument/equals_test - ns/op	9107 ns/op	7711 ns/op	1.18
BenchmarkDocument/equals_test - B/op	7273 B/op	7273 B/op	1
BenchmarkDocument/equals_test - allocs/op	132 allocs/op	132 allocs/op	1
BenchmarkDocument/nested_update_test	18644 ns/op 12139 B/op 262 allocs/op	16885 ns/op 12138 B/op 262 allocs/op	1.10
BenchmarkDocument/nested_update_test - ns/op	18644 ns/op	16885 ns/op	1.10
BenchmarkDocument/nested_update_test - B/op	12139 B/op	12138 B/op	1.00
BenchmarkDocument/nested_update_test - allocs/op	262 allocs/op	262 allocs/op	1
BenchmarkDocument/delete_test	23834 ns/op 15363 B/op 341 allocs/op	22814 ns/op 15363 B/op 341 allocs/op	1.04
BenchmarkDocument/delete_test - ns/op	23834 ns/op	22814 ns/op	1.04
BenchmarkDocument/delete_test - B/op	15363 B/op	15363 B/op	1
BenchmarkDocument/delete_test - allocs/op	341 allocs/op	341 allocs/op	1
BenchmarkDocument/object_test	8800 ns/op 6817 B/op 120 allocs/op	8655 ns/op 6817 B/op 120 allocs/op	1.02
BenchmarkDocument/object_test - ns/op	8800 ns/op	8655 ns/op	1.02
BenchmarkDocument/object_test - B/op	6817 B/op	6817 B/op	1
BenchmarkDocument/object_test - allocs/op	120 allocs/op	120 allocs/op	1
BenchmarkDocument/array_test	30854 ns/op 11947 B/op 276 allocs/op	29485 ns/op 11947 B/op 276 allocs/op	1.05
BenchmarkDocument/array_test - ns/op	30854 ns/op	29485 ns/op	1.05
BenchmarkDocument/array_test - B/op	11947 B/op	11947 B/op	1
BenchmarkDocument/array_test - allocs/op	276 allocs/op	276 allocs/op	1
BenchmarkDocument/text_test	31200 ns/op 14716 B/op 469 allocs/op	30626 ns/op 14715 B/op 469 allocs/op	1.02
BenchmarkDocument/text_test - ns/op	31200 ns/op	30626 ns/op	1.02
BenchmarkDocument/text_test - B/op	14716 B/op	14715 B/op	1.00
BenchmarkDocument/text_test - allocs/op	469 allocs/op	469 allocs/op	1
BenchmarkDocument/text_composition_test	30189 ns/op 18423 B/op 484 allocs/op	29264 ns/op 18422 B/op 484 allocs/op	1.03
BenchmarkDocument/text_composition_test - ns/op	30189 ns/op	29264 ns/op	1.03
BenchmarkDocument/text_composition_test - B/op	18423 B/op	18422 B/op	1.00
BenchmarkDocument/text_composition_test - allocs/op	484 allocs/op	484 allocs/op	1
BenchmarkDocument/rich_text_test	87527 ns/op 38477 B/op 1148 allocs/op	80818 ns/op 38477 B/op 1148 allocs/op	1.08
BenchmarkDocument/rich_text_test - ns/op	87527 ns/op	80818 ns/op	1.08
BenchmarkDocument/rich_text_test - B/op	38477 B/op	38477 B/op	1
BenchmarkDocument/rich_text_test - allocs/op	1148 allocs/op	1148 allocs/op	1
BenchmarkDocument/counter_test	18052 ns/op 10722 B/op 244 allocs/op	17410 ns/op 10722 B/op 244 allocs/op	1.04
BenchmarkDocument/counter_test - ns/op	18052 ns/op	17410 ns/op	1.04
BenchmarkDocument/counter_test - B/op	10722 B/op	10722 B/op	1
BenchmarkDocument/counter_test - allocs/op	244 allocs/op	244 allocs/op	1
BenchmarkDocument/text_edit_gc_100	1322603 ns/op 870928 B/op 16753 allocs/op	1286023 ns/op 870966 B/op 16752 allocs/op	1.03
BenchmarkDocument/text_edit_gc_100 - ns/op	1322603 ns/op	1286023 ns/op	1.03
BenchmarkDocument/text_edit_gc_100 - B/op	870928 B/op	870966 B/op	1.00
BenchmarkDocument/text_edit_gc_100 - allocs/op	16753 allocs/op	16752 allocs/op	1.00
BenchmarkDocument/text_edit_gc_1000	52561032 ns/op 50535742 B/op 181706 allocs/op	49907328 ns/op 50536573 B/op 181721 allocs/op	1.05
BenchmarkDocument/text_edit_gc_1000 - ns/op	52561032 ns/op	49907328 ns/op	1.05
BenchmarkDocument/text_edit_gc_1000 - B/op	50535742 B/op	50536573 B/op	1.00
BenchmarkDocument/text_edit_gc_1000 - allocs/op	181706 allocs/op	181721 allocs/op	1.00
BenchmarkDocument/text_split_gc_100	1923191 ns/op 1528812 B/op 15605 allocs/op	1874424 ns/op 1528858 B/op 15604 allocs/op	1.03
BenchmarkDocument/text_split_gc_100 - ns/op	1923191 ns/op	1874424 ns/op	1.03
BenchmarkDocument/text_split_gc_100 - B/op	1528812 B/op	1528858 B/op	1.00
BenchmarkDocument/text_split_gc_100 - allocs/op	15605 allocs/op	15604 allocs/op	1.00
BenchmarkDocument/text_split_gc_1000	116163343 ns/op 135076884 B/op 182194 allocs/op	112206298 ns/op 135077490 B/op 182205 allocs/op	1.04
BenchmarkDocument/text_split_gc_1000 - ns/op	116163343 ns/op	112206298 ns/op	1.04
BenchmarkDocument/text_split_gc_1000 - B/op	135076884 B/op	135077490 B/op	1.00
BenchmarkDocument/text_split_gc_1000 - allocs/op	182194 allocs/op	182205 allocs/op	1.00
BenchmarkDocument/text_delete_all_10000	17011942 ns/op 10182740 B/op 40675 allocs/op	16311336 ns/op 10184912 B/op 40680 allocs/op	1.04
BenchmarkDocument/text_delete_all_10000 - ns/op	17011942 ns/op	16311336 ns/op	1.04
BenchmarkDocument/text_delete_all_10000 - B/op	10182740 B/op	10184912 B/op	1.00
BenchmarkDocument/text_delete_all_10000 - allocs/op	40675 allocs/op	40680 allocs/op	1.00
BenchmarkDocument/text_delete_all_100000	335371278 ns/op 142665221 B/op 411702 allocs/op	302973366 ns/op 142701236 B/op 411736 allocs/op	1.11
BenchmarkDocument/text_delete_all_100000 - ns/op	335371278 ns/op	302973366 ns/op	1.11
BenchmarkDocument/text_delete_all_100000 - B/op	142665221 B/op	142701236 B/op	1.00
BenchmarkDocument/text_delete_all_100000 - allocs/op	411702 allocs/op	411736 allocs/op	1.00
BenchmarkDocument/text_100	228029 ns/op 120038 B/op 5081 allocs/op	230800 ns/op 120037 B/op 5081 allocs/op	0.99
BenchmarkDocument/text_100 - ns/op	228029 ns/op	230800 ns/op	0.99
BenchmarkDocument/text_100 - B/op	120038 B/op	120037 B/op	1.00
BenchmarkDocument/text_100 - allocs/op	5081 allocs/op	5081 allocs/op	1
BenchmarkDocument/text_1000	2546211 ns/op 1169027 B/op 50085 allocs/op	2432389 ns/op 1169009 B/op 50085 allocs/op	1.05
BenchmarkDocument/text_1000 - ns/op	2546211 ns/op	2432389 ns/op	1.05
BenchmarkDocument/text_1000 - B/op	1169027 B/op	1169009 B/op	1.00
BenchmarkDocument/text_1000 - allocs/op	50085 allocs/op	50085 allocs/op	1
BenchmarkDocument/array_1000	1312345 ns/op 1091455 B/op 11832 allocs/op	1270037 ns/op 1091354 B/op 11831 allocs/op	1.03
BenchmarkDocument/array_1000 - ns/op	1312345 ns/op	1270037 ns/op	1.03
BenchmarkDocument/array_1000 - B/op	1091455 B/op	1091354 B/op	1.00
BenchmarkDocument/array_1000 - allocs/op	11832 allocs/op	11831 allocs/op	1.00
BenchmarkDocument/array_10000	13165771 ns/op 9800049 B/op 120296 allocs/op	13137945 ns/op 9799739 B/op 120295 allocs/op	1.00
BenchmarkDocument/array_10000 - ns/op	13165771 ns/op	13137945 ns/op	1.00
BenchmarkDocument/array_10000 - B/op	9800049 B/op	9799739 B/op	1.00
BenchmarkDocument/array_10000 - allocs/op	120296 allocs/op	120295 allocs/op	1.00
BenchmarkDocument/array_gc_100	158765 ns/op 132719 B/op 1260 allocs/op	156391 ns/op 132721 B/op 1260 allocs/op	1.02
BenchmarkDocument/array_gc_100 - ns/op	158765 ns/op	156391 ns/op	1.02
BenchmarkDocument/array_gc_100 - B/op	132719 B/op	132721 B/op	1.00
BenchmarkDocument/array_gc_100 - allocs/op	1260 allocs/op	1260 allocs/op	1
BenchmarkDocument/array_gc_1000	1465462 ns/op 1159182 B/op 12877 allocs/op	1453233 ns/op 1159078 B/op 12876 allocs/op	1.01
BenchmarkDocument/array_gc_1000 - ns/op	1465462 ns/op	1453233 ns/op	1.01
BenchmarkDocument/array_gc_1000 - B/op	1159182 B/op	1159078 B/op	1.00
BenchmarkDocument/array_gc_1000 - allocs/op	12877 allocs/op	12876 allocs/op	1.00
BenchmarkDocument/counter_1000	215340 ns/op 193081 B/op 5771 allocs/op	213589 ns/op 193080 B/op 5771 allocs/op	1.01
BenchmarkDocument/counter_1000 - ns/op	215340 ns/op	213589 ns/op	1.01
BenchmarkDocument/counter_1000 - B/op	193081 B/op	193080 B/op	1.00
BenchmarkDocument/counter_1000 - allocs/op	5771 allocs/op	5771 allocs/op	1
BenchmarkDocument/counter_10000	2252781 ns/op 2087998 B/op 59778 allocs/op	2232275 ns/op 2087996 B/op 59778 allocs/op	1.01
BenchmarkDocument/counter_10000 - ns/op	2252781 ns/op	2232275 ns/op	1.01
BenchmarkDocument/counter_10000 - B/op	2087998 B/op	2087996 B/op	1.00
BenchmarkDocument/counter_10000 - allocs/op	59778 allocs/op	59778 allocs/op	1
BenchmarkDocument/object_1000	1484762 ns/op 1428152 B/op 9849 allocs/op	1445068 ns/op 1428164 B/op 9849 allocs/op	1.03
BenchmarkDocument/object_1000 - ns/op	1484762 ns/op	1445068 ns/op	1.03
BenchmarkDocument/object_1000 - B/op	1428152 B/op	1428164 B/op	1.00
BenchmarkDocument/object_1000 - allocs/op	9849 allocs/op	9849 allocs/op	1
BenchmarkDocument/object_10000	15899246 ns/op 12168314 B/op 100569 allocs/op	15725862 ns/op 12168058 B/op 100568 allocs/op	1.01
BenchmarkDocument/object_10000 - ns/op	15899246 ns/op	15725862 ns/op	1.01
BenchmarkDocument/object_10000 - B/op	12168314 B/op	12168058 B/op	1.00
BenchmarkDocument/object_10000 - allocs/op	100569 allocs/op	100568 allocs/op	1.00
BenchmarkDocument/tree_100	1089626 ns/op 943703 B/op 6101 allocs/op	1065747 ns/op 943703 B/op 6101 allocs/op	1.02
BenchmarkDocument/tree_100 - ns/op	1089626 ns/op	1065747 ns/op	1.02
BenchmarkDocument/tree_100 - B/op	943703 B/op	943703 B/op	1
BenchmarkDocument/tree_100 - allocs/op	6101 allocs/op	6101 allocs/op	1
BenchmarkDocument/tree_1000	81743244 ns/op 86460488 B/op 60115 allocs/op	79611023 ns/op 86460432 B/op 60114 allocs/op	1.03
BenchmarkDocument/tree_1000 - ns/op	81743244 ns/op	79611023 ns/op	1.03
BenchmarkDocument/tree_1000 - B/op	86460488 B/op	86460432 B/op	1.00
BenchmarkDocument/tree_1000 - allocs/op	60115 allocs/op	60114 allocs/op	1.00
BenchmarkDocument/tree_10000	10375094895 ns/op 8580666048 B/op 600207 allocs/op	9896720415 ns/op 8580660416 B/op 600221 allocs/op	1.05
BenchmarkDocument/tree_10000 - ns/op	10375094895 ns/op	9896720415 ns/op	1.05
BenchmarkDocument/tree_10000 - B/op	8580666048 B/op	8580660416 B/op	1.00
BenchmarkDocument/tree_10000 - allocs/op	600207 allocs/op	600221 allocs/op	1.00
BenchmarkDocument/tree_delete_all_1000	82216921 ns/op 87508640 B/op 75264 allocs/op	80110159 ns/op 87511336 B/op 75269 allocs/op	1.03
BenchmarkDocument/tree_delete_all_1000 - ns/op	82216921 ns/op	80110159 ns/op	1.03
BenchmarkDocument/tree_delete_all_1000 - B/op	87508640 B/op	87511336 B/op	1.00
BenchmarkDocument/tree_delete_all_1000 - allocs/op	75264 allocs/op	75269 allocs/op	1.00
BenchmarkDocument/tree_edit_gc_100	4147721 ns/op 4146725 B/op 15141 allocs/op	4032926 ns/op 4146751 B/op 15141 allocs/op	1.03
BenchmarkDocument/tree_edit_gc_100 - ns/op	4147721 ns/op	4032926 ns/op	1.03
BenchmarkDocument/tree_edit_gc_100 - B/op	4146725 B/op	4146751 B/op	1.00
BenchmarkDocument/tree_edit_gc_100 - allocs/op	15141 allocs/op	15141 allocs/op	1
BenchmarkDocument/tree_edit_gc_1000	337162143 ns/op 383749208 B/op 154874 allocs/op	315805156 ns/op 383747440 B/op 154870 allocs/op	1.07
BenchmarkDocument/tree_edit_gc_1000 - ns/op	337162143 ns/op	315805156 ns/op	1.07
BenchmarkDocument/tree_edit_gc_1000 - B/op	383749208 B/op	383747440 B/op	1.00
BenchmarkDocument/tree_edit_gc_1000 - allocs/op	154874 allocs/op	154870 allocs/op	1.00
BenchmarkDocument/tree_split_gc_100	2745652 ns/op 2412584 B/op 11125 allocs/op	2665509 ns/op 2412628 B/op 11126 allocs/op	1.03
BenchmarkDocument/tree_split_gc_100 - ns/op	2745652 ns/op	2665509 ns/op	1.03
BenchmarkDocument/tree_split_gc_100 - B/op	2412584 B/op	2412628 B/op	1.00
BenchmarkDocument/tree_split_gc_100 - allocs/op	11125 allocs/op	11126 allocs/op	1.00
BenchmarkDocument/tree_split_gc_1000	200946594 ns/op 222253384 B/op 122012 allocs/op	194354802 ns/op 222252425 B/op 121997 allocs/op	1.03
BenchmarkDocument/tree_split_gc_1000 - ns/op	200946594 ns/op	194354802 ns/op	1.03
BenchmarkDocument/tree_split_gc_1000 - B/op	222253384 B/op	222252425 B/op	1.00
BenchmarkDocument/tree_split_gc_1000 - allocs/op	122012 allocs/op	121997 allocs/op	1.00
BenchmarkRPC/client_to_server	394155102 ns/op 16972877 B/op 175379 allocs/op	384662242 ns/op 17778757 B/op 175400 allocs/op	1.02
BenchmarkRPC/client_to_server - ns/op	394155102 ns/op	384662242 ns/op	1.02
BenchmarkRPC/client_to_server - B/op	16972877 B/op	17778757 B/op	0.95
BenchmarkRPC/client_to_server - allocs/op	175379 allocs/op	175400 allocs/op	1.00
BenchmarkRPC/client_to_client_via_server	639182764 ns/op 32266644 B/op 321497 allocs/op	636129150 ns/op 32619796 B/op 320977 allocs/op	1.00
BenchmarkRPC/client_to_client_via_server - ns/op	639182764 ns/op	636129150 ns/op	1.00
BenchmarkRPC/client_to_client_via_server - B/op	32266644 B/op	32619796 B/op	0.99
BenchmarkRPC/client_to_client_via_server - allocs/op	321497 allocs/op	320977 allocs/op	1.00
BenchmarkRPC/attach_large_document	1257688131 ns/op 1896304752 B/op 8910 allocs/op	1275518714 ns/op 1884974208 B/op 8848 allocs/op	0.99
BenchmarkRPC/attach_large_document - ns/op	1257688131 ns/op	1275518714 ns/op	0.99
BenchmarkRPC/attach_large_document - B/op	1896304752 B/op	1884974208 B/op	1.01
BenchmarkRPC/attach_large_document - allocs/op	8910 allocs/op	8848 allocs/op	1.01
BenchmarkRPC/adminCli_to_server	552832443 ns/op 35955536 B/op 289550 allocs/op	549596252 ns/op 35960076 B/op 289556 allocs/op	1.01
BenchmarkRPC/adminCli_to_server - ns/op	552832443 ns/op	549596252 ns/op	1.01
BenchmarkRPC/adminCli_to_server - B/op	35955536 B/op	35960076 B/op	1.00
BenchmarkRPC/adminCli_to_server - allocs/op	289550 allocs/op	289556 allocs/op	1.00
BenchmarkLocker	63.94 ns/op 16 B/op 1 allocs/op	64.11 ns/op 16 B/op 1 allocs/op	1.00
BenchmarkLocker - ns/op	63.94 ns/op	64.11 ns/op	1.00
BenchmarkLocker - B/op	16 B/op	16 B/op	1
BenchmarkLocker - allocs/op	1 allocs/op	1 allocs/op	1
BenchmarkLockerParallel	38.99 ns/op 0 B/op 0 allocs/op	38.8 ns/op 0 B/op 0 allocs/op	1.00
BenchmarkLockerParallel - ns/op	38.99 ns/op	38.8 ns/op	1.00
BenchmarkLockerParallel - B/op	0 B/op	0 B/op	1
BenchmarkLockerParallel - allocs/op	0 allocs/op	0 allocs/op	1
BenchmarkLockerMoreKeys	154.7 ns/op 15 B/op 0 allocs/op	152.9 ns/op 15 B/op 0 allocs/op	1.01
BenchmarkLockerMoreKeys - ns/op	154.7 ns/op	152.9 ns/op	1.01
BenchmarkLockerMoreKeys - B/op	15 B/op	15 B/op	1
BenchmarkLockerMoreKeys - allocs/op	0 allocs/op	0 allocs/op	1
BenchmarkChange/Push_10_Changes	3970326 ns/op 121367 B/op 1284 allocs/op	3918599 ns/op 121228 B/op 1284 allocs/op	1.01
BenchmarkChange/Push_10_Changes - ns/op	3970326 ns/op	3918599 ns/op	1.01
BenchmarkChange/Push_10_Changes - B/op	121367 B/op	121228 B/op	1.00
BenchmarkChange/Push_10_Changes - allocs/op	1284 allocs/op	1284 allocs/op	1
BenchmarkChange/Push_100_Changes	14925994 ns/op 571630 B/op 6655 allocs/op	14644289 ns/op 569826 B/op 6654 allocs/op	1.02
BenchmarkChange/Push_100_Changes - ns/op	14925994 ns/op	14644289 ns/op	1.02
BenchmarkChange/Push_100_Changes - B/op	571630 B/op	569826 B/op	1.00
BenchmarkChange/Push_100_Changes - allocs/op	6655 allocs/op	6654 allocs/op	1.00
BenchmarkChange/Push_1000_Changes	119501990 ns/op 5273322 B/op 63147 allocs/op	117618328 ns/op 5334226 B/op 63147 allocs/op	1.02
BenchmarkChange/Push_1000_Changes - ns/op	119501990 ns/op	117618328 ns/op	1.02
BenchmarkChange/Push_1000_Changes - B/op	5273322 B/op	5334226 B/op	0.99
BenchmarkChange/Push_1000_Changes - allocs/op	63147 allocs/op	63147 allocs/op	1
BenchmarkChange/Pull_10_Changes	3389155 ns/op 98328 B/op 1004 allocs/op	2924509 ns/op 100678 B/op 1004 allocs/op	1.16
BenchmarkChange/Pull_10_Changes - ns/op	3389155 ns/op	2924509 ns/op	1.16
BenchmarkChange/Pull_10_Changes - B/op	98328 B/op	100678 B/op	0.98
BenchmarkChange/Pull_10_Changes - allocs/op	1004 allocs/op	1004 allocs/op	1
BenchmarkChange/Pull_100_Changes	4416693 ns/op 261446 B/op 3475 allocs/op	4373249 ns/op 265852 B/op 3475 allocs/op	1.01
BenchmarkChange/Pull_100_Changes - ns/op	4416693 ns/op	4373249 ns/op	1.01
BenchmarkChange/Pull_100_Changes - B/op	261446 B/op	265852 B/op	0.98
BenchmarkChange/Pull_100_Changes - allocs/op	3475 allocs/op	3475 allocs/op	1
BenchmarkChange/Pull_1000_Changes	9098680 ns/op 1486201 B/op 29847 allocs/op	8776890 ns/op 1490384 B/op 29857 allocs/op	1.04
BenchmarkChange/Pull_1000_Changes - ns/op	9098680 ns/op	8776890 ns/op	1.04
BenchmarkChange/Pull_1000_Changes - B/op	1486201 B/op	1490384 B/op	1.00
BenchmarkChange/Pull_1000_Changes - allocs/op	29847 allocs/op	29857 allocs/op	1.00
BenchmarkSnapshot/Push_3KB_snapshot	17733995 ns/op 707169 B/op 6656 allocs/op	17132822 ns/op 713528 B/op 6656 allocs/op	1.04
BenchmarkSnapshot/Push_3KB_snapshot - ns/op	17733995 ns/op	17132822 ns/op	1.04
BenchmarkSnapshot/Push_3KB_snapshot - B/op	707169 B/op	713528 B/op	0.99
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op	6656 allocs/op	6656 allocs/op	1
BenchmarkSnapshot/Push_30KB_snapshot	122531312 ns/op 5599786 B/op 63295 allocs/op	120733528 ns/op 5517276 B/op 63146 allocs/op	1.01
BenchmarkSnapshot/Push_30KB_snapshot - ns/op	122531312 ns/op	120733528 ns/op	1.01
BenchmarkSnapshot/Push_30KB_snapshot - B/op	5599786 B/op	5517276 B/op	1.01
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op	63295 allocs/op	63146 allocs/op	1.00
BenchmarkSnapshot/Pull_3KB_snapshot	7015133 ns/op 914879 B/op 15500 allocs/op	6476718 ns/op 922184 B/op 15513 allocs/op	1.08
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op	7015133 ns/op	6476718 ns/op	1.08
BenchmarkSnapshot/Pull_3KB_snapshot - B/op	914879 B/op	922184 B/op	0.99
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op	15500 allocs/op	15513 allocs/op	1.00
BenchmarkSnapshot/Pull_30KB_snapshot	15715900 ns/op 7152344 B/op 150105 allocs/op	15301604 ns/op 7152748 B/op 150103 allocs/op	1.03
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op	15715900 ns/op	15301604 ns/op	1.03
BenchmarkSnapshot/Pull_30KB_snapshot - B/op	7152344 B/op	7152748 B/op	1.00
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op	150105 allocs/op	150103 allocs/op	1.00
BenchmarkSync/memory_sync_10_test	6910 ns/op 1286 B/op 38 allocs/op	6691 ns/op 1286 B/op 38 allocs/op	1.03
BenchmarkSync/memory_sync_10_test - ns/op	6910 ns/op	6691 ns/op	1.03
BenchmarkSync/memory_sync_10_test - B/op	1286 B/op	1286 B/op	1
BenchmarkSync/memory_sync_10_test - allocs/op	38 allocs/op	38 allocs/op	1
BenchmarkSync/memory_sync_100_test	51459 ns/op 8692 B/op 276 allocs/op	50725 ns/op 8648 B/op 273 allocs/op	1.01
BenchmarkSync/memory_sync_100_test - ns/op	51459 ns/op	50725 ns/op	1.01
BenchmarkSync/memory_sync_100_test - B/op	8692 B/op	8648 B/op	1.01
BenchmarkSync/memory_sync_100_test - allocs/op	276 allocs/op	273 allocs/op	1.01
BenchmarkSync/memory_sync_1000_test	595336 ns/op 74706 B/op 2145 allocs/op	584946 ns/op 74189 B/op 2110 allocs/op	1.02
BenchmarkSync/memory_sync_1000_test - ns/op	595336 ns/op	584946 ns/op	1.02
BenchmarkSync/memory_sync_1000_test - B/op	74706 B/op	74189 B/op	1.01
BenchmarkSync/memory_sync_1000_test - allocs/op	2145 allocs/op	2110 allocs/op	1.02
BenchmarkSync/memory_sync_10000_test	7466475 ns/op 739305 B/op 20270 allocs/op	7295562 ns/op 737626 B/op 20312 allocs/op	1.02
BenchmarkSync/memory_sync_10000_test - ns/op	7466475 ns/op	7295562 ns/op	1.02
BenchmarkSync/memory_sync_10000_test - B/op	739305 B/op	737626 B/op	1.00
BenchmarkSync/memory_sync_10000_test - allocs/op	20270 allocs/op	20312 allocs/op	1.00
BenchmarkTextEditing	5390196880 ns/op 3901914656 B/op 18743176 allocs/op	4871254002 ns/op 3901928048 B/op 18743223 allocs/op	1.11
BenchmarkTextEditing - ns/op	5390196880 ns/op	4871254002 ns/op	1.11
BenchmarkTextEditing - B/op	3901914656 B/op	3901928048 B/op	1.00
BenchmarkTextEditing - allocs/op	18743176 allocs/op	18743223 allocs/op	1.00
BenchmarkTree/10000_vertices_to_protobuf	3557213 ns/op 6262974 B/op 70025 allocs/op	3515077 ns/op 6262996 B/op 70025 allocs/op	1.01
BenchmarkTree/10000_vertices_to_protobuf - ns/op	3557213 ns/op	3515077 ns/op	1.01
BenchmarkTree/10000_vertices_to_protobuf - B/op	6262974 B/op	6262996 B/op	1.00
BenchmarkTree/10000_vertices_to_protobuf - allocs/op	70025 allocs/op	70025 allocs/op	1
BenchmarkTree/10000_vertices_from_protobuf	164134765 ns/op 442172701 B/op 290051 allocs/op	156877583 ns/op 442171445 B/op 290039 allocs/op	1.05
BenchmarkTree/10000_vertices_from_protobuf - ns/op	164134765 ns/op	156877583 ns/op	1.05
BenchmarkTree/10000_vertices_from_protobuf - B/op	442172701 B/op	442171445 B/op	1.00
BenchmarkTree/10000_vertices_from_protobuf - allocs/op	290051 allocs/op	290039 allocs/op	1.00
BenchmarkTree/20000_vertices_to_protobuf	7735042 ns/op 12717038 B/op 140028 allocs/op	7704529 ns/op 12717040 B/op 140028 allocs/op	1.00
BenchmarkTree/20000_vertices_to_protobuf - ns/op	7735042 ns/op	7704529 ns/op	1.00
BenchmarkTree/20000_vertices_to_protobuf - B/op	12717038 B/op	12717040 B/op	1.00
BenchmarkTree/20000_vertices_to_protobuf - allocs/op	140028 allocs/op	140028 allocs/op	1
BenchmarkTree/20000_vertices_from_protobuf	705552666 ns/op 1697271896 B/op 580043 allocs/op	705631557 ns/op 1697267800 B/op 580042 allocs/op	1.00
BenchmarkTree/20000_vertices_from_protobuf - ns/op	705552666 ns/op	705631557 ns/op	1.00
BenchmarkTree/20000_vertices_from_protobuf - B/op	1697271896 B/op	1697267800 B/op	1.00
BenchmarkTree/20000_vertices_from_protobuf - allocs/op	580043 allocs/op	580042 allocs/op	1.00
BenchmarkTree/30000_vertices_to_protobuf	12218861 ns/op 19318246 B/op 210030 allocs/op	12574811 ns/op 19318412 B/op 210030 allocs/op	0.97
BenchmarkTree/30000_vertices_to_protobuf - ns/op	12218861 ns/op	12574811 ns/op	0.97
BenchmarkTree/30000_vertices_to_protobuf - B/op	19318246 B/op	19318412 B/op	1.00
BenchmarkTree/30000_vertices_to_protobuf - allocs/op	210030 allocs/op	210030 allocs/op	1
BenchmarkTree/30000_vertices_from_protobuf	1665770551 ns/op 3752053432 B/op 870056 allocs/op	1634388807 ns/op 3752052120 B/op 870047 allocs/op	1.02
BenchmarkTree/30000_vertices_from_protobuf - ns/op	1665770551 ns/op	1634388807 ns/op	1.02
BenchmarkTree/30000_vertices_from_protobuf - B/op	3752053432 B/op	3752052120 B/op	1.00
BenchmarkTree/30000_vertices_from_protobuf - allocs/op	870056 allocs/op	870047 allocs/op	1.00

This comment was automatically generated by workflow using github-action-benchmark.

[binary-ho]

hi @sejongk here is the description of additional codes

1. add validation Change's ClientSeq differs by 1 from the ClientSeq in the existing docInfo

// packs/packs.go
func validateClientSeqSequentialWithCheckpoint(changes []*change.Change, checkpoint change.Checkpoint) error {
	expectedClientSeq := checkpoint.ClientSeq + 1
	actualFirstClientSeq := changes[0].ClientSeq()

	if expectedClientSeq < actualFirstClientSeq {
		return fmt.Errorf(
			"ClientSeq is not sequential with DocInfo.Checkpoint.ClientSeq (expected: %d, actual: %d) : %w",
			expectedClientSeq,
			actualFirstClientSeq,
			ErrClientSeqNotSequentialWithCheckpoint,
		)
	}
	return nil
}

and here is full validation codes

func validateClientSeqSequential(changes []*change.Change, checkpoint change.Checkpoint) error {
	if len(changes) < 1 {
		return nil
	}

	if err := validateClientSeqSequentialWithCheckpoint(changes, checkpoint); err != nil {
		return err
	}

	return validateClientSeqInChangesAreSequential(changes)
}

func validateClientSeqSequentialWithCheckpoint(changes []*change.Change, checkpoint change.Checkpoint) error {
	expectedClientSeq := checkpoint.ClientSeq + 1
	actualFirstClientSeq := changes[0].ClientSeq()

	if expectedClientSeq < actualFirstClientSeq {
		return fmt.Errorf(
			"ClientSeq is not sequential with DocInfo.Checkpoint.ClientSeq (expected: %d, actual: %d) : %w",
			expectedClientSeq,
			actualFirstClientSeq,
			ErrClientSeqNotSequentialWithCheckpoint,
		)
	}
	return nil
}

func validateClientSeqInChangesAreSequential(changes []*change.Change) error {
	nextClientSeq := changes[0].ClientSeq()
	for _, cn := range changes[1:] {
		nextClientSeq++

		if nextClientSeq != cn.ClientSeq() {
			return fmt.Errorf(
				"ClientSeq in Changes are not sequential (expected: %d, actual: %d) : %w",
				nextClientSeq,
				cn.ClientSeq(),
				ErrClientSeqInChangesAreNotSequential,
			)
		}
	}
	return nil
}

2. Seperate ErrClientSeqNotSequential to two error case

1. ErrClientSeqNotSequentialWithCheckpoint
2. ErrClientSeqInChangesAreNotSequential

3. And Write Additional Test Codes

// packs/packs_test.go

func RunPushPullWithNotSequentialClientSeqWithCheckpoint(t *testing.T) {
	ctx := context.Background()
	be := setUpBackend(t)
	project, _ := be.DB.FindProjectInfoByID(
		ctx,
		database.DefaultProjectID,
	)

	clientInfo, _ := clients.Activate(ctx, be.DB, project.ToProject(), clientID)

	actorID, _ := time.ActorIDFromHex(clientID)
	changePackFixture, _ :=
		createChangePackFixture(helper.TestDocKey(t).String(), actorID.Bytes())

	docInfo, _ := documents.FindDocInfoByKeyAndOwner(
		ctx, be, clientInfo, changePackFixture.DocumentKey, true)
	err := clientInfo.AttachDocument(docInfo.ID, changePackFixture.IsAttached())
	if err != nil {
		assert.Fail(t, "failed to attach document")
	}

	_, err = packs.PushPull(ctx, be, project.ToProject(),
		clientInfo, docInfo, changePackFixture, packs.PushPullOptions{
			Mode:   types.SyncModePushPull,
			Status: document.StatusAttached,
		})
	if err != nil {
		assert.Fail(t, "failed to push pull")
	}

	changePackWithNotSequentialClientSeqWithCheckpoint, _ :=
		createChangePackWithNotSequentialClientSeqWithCheckpoint(helper.TestDocKey(t).String(), actorID.Bytes())
	_, err = packs.PushPull(ctx, be, project.ToProject(), clientInfo, docInfo,
		changePackWithNotSequentialClientSeqWithCheckpoint, packs.PushPullOptions{
			Mode:   types.SyncModePushPull,
			Status: document.StatusAttached,
		})
	assert.Equal(t, connecthelper.CodeOf(packs.ErrClientSeqNotSequentialWithCheckpoint), connecthelper.CodeOf(err))
}

4. And Remove the TODO comments

i remove the TODO comments into packs/packs.go, function PushPull

// TODO: Changes may be reordered or missing during communication on the network.
// We should check the change.pack with checkpoint to make sure the changes are in the correct order.	

[binary-ho]

I changed it to return the CodeFailedPrecondition status code to the client in the case of ErrClientSeqNotSequentialWithCheckpoint.

Why?

@sejongk asked me whether it is appropriate for both ErrClientSeqNotSequentialWithCheckpoint and ErrClientSeqInChangesAreNotSequential to return InvalidArgument.

so, I read the gRPC Core - Status codes and their use in gRPC documentation.

and Here is an excerpt from the document:

1. INVALID_ARGUMENT: The client specified an invalid argument.
   Note that this differs from FAILED_PRECONDITION. INVALID_ARGUMENT indicates arguments that are problematic regardless of the state of the system (e.g., a malformed file name).
   
2. FAILED_PRECONDITION: The operation was rejected because the system is not in a state required for the operation's execution.
   For example, the directory to be deleted is non-empty, an rmdir operation is applied to a non-directory, etc.

The difference lies in whether or not it is related to the system's status.

After reading the document,
I concluded that it would be more appropriate for the two errors to return different status codes:

1. ErrClientSeqNotSequentialWithCheckpoint -> FailedPrecondition
2. ErrClientSeqInChangesAreNotSequential -> InvalidArgument

As I understand it:

1. INVALID_ARGUMENT is an error that occurs when an argument falls outside the predefined "correct argument" -> regardless of the system's "status."
2. FailedPrecondition is an error that can occur -> depending on the system's current "status."

The examples provided in the document align with this understanding:

1. INVALID_ARGUMENT: "The format of the file passed as an argument is incorrect" -> This is independent of the system's current status.
2. FailedPrecondition: "The rmdir operation cannot be executed because the directory is not empty" -> This is related to the current "status" of the directory. If the directory is empty, no error occurs, but if it is not empty, an error is triggered.

In the case of ErrClientSeqNotSequentialWithCheckpoint, the error occurrence depends on the current state of the ClientSeq in the Checkpoint of ClientInfo. This is related to the system's "status."

On the other hand, the requirement that "ClientSeqs within Changes must be sequential" in ErrClientSeqInChangesAreNotSequential is unrelated to the server system's status. It only compares the arguments. The "correct argument" is defined, and it simply checks if the arguments conform. This is independent of the system's status.

Conclusion

Therefore, I believe it is better for these two situations to return different status codes:

1. ErrClientSeqNotSequentialWithCheckpoint -> FailedPrecondition
2. ErrClientSeqInChangesAreNotSequential -> InvalidArgument