Fixes #5001 #5078

dviejokfs · 2024-12-17T11:56:29Z

Type of change

Improvement (improvement to code, performance, etc)

Description

This pull request introduces several improvements in the HandleGetStateByRange function within the core/chaincode/handler.go file and the GetStateRangeScanIteratorWithPagination function within the core/ledger/kvledger/txmgmt/statedb/stateleveldb/stateleveldb.go file. The changes include:

Adding panic recovery in the HandleGetStateByRange function to log errors and clean up any query contexts that may have been created.
Validating start and end keys to ensure they are not both empty.
Adding validation for the page size to ensure it is greater than zero.
Cleaning up query contexts if errors occur during the range query or if the range query iterator is nil.
Wrapping the iterator creation in a recover block in GetStateRangeScanIteratorWithPagination to handle potential panics and return meaningful error messages.
Ensuring that a nil iterator is not returned from the database.

Additional details

These changes help improve the robustness of the code by handling potential panics and validating inputs, which can help prevent unexpected errors and improve the overall stability of the system.

Related issues

#5001

Signed-off-by: David VIEJO <[email protected]>

This commit introduces the createResponse function in handler.go, which constructs a QueryResponse for range queries. It handles pagination and includes error handling for payload marshaling, ensuring proper cleanup of the query context in case of errors. Signed-off-by: David VIEJO <[email protected]>

Signed-off-by: David VIEJO <[email protected]>

yacovm · 2024-12-17T12:54:46Z

gossip/util/misc.go

-	var seed [32]byte
-	_, _ = crand.Read(seed[:])
-	r = rand.New(rand.NewChaCha8(seed))
+	r = rand.New(rand.NewSource(time.Now().UnixNano()))


why would we seed this with a deterministic and non-random seed?
what was before was better, in my opinion.

ok, rolling back

yacovm · 2024-12-17T12:56:41Z

gossip/gossip/algo/pull.go

 			return n
 		}
 	}
+


We shouldn't try 100 attempts, but more attempts, like 1000,000.

and if we still fail, then we should loop until we find a nonce that is not in use using the crypto rand.

For the second part it can result in slow nonces if this happens right?

after looking at the code myself, I think we can just use util.RandomUInt64 until it no longer exists in the outgoing nonces.

yacovm · 2024-12-17T12:58:28Z

I don't understand how the gossip nonce generation is related to the panic, can you explain?

pfi79 · 2024-12-17T12:59:44Z

gossip/util/misc.go

-	"math/rand/v2"
+	"math/rand"


Why did you change the library from v2 to the previous library?

sorry, didn't mean to, changed back

Signed-off-by: David VIEJO <[email protected]>

- Corrected comment capitalization for the HandleRegister function in handler.go. - Updated the import path for the math/rand package to math/rand/v2 in misc.go. These changes improve code readability and maintain consistency in import statements. Signed-off-by: David VIEJO <[email protected]>

This commit introduces the import of the crypto/rand package in misc.go, which may be utilized for secure random number generation. This addition complements the existing math/rand/v2 import and prepares the code for future enhancements requiring cryptographic randomness. Signed-off-by: David VIEJO <[email protected]>

This commit corrects the function name from `Intn` to `IntN` in the RandomInt function, ensuring consistency with the updated import path for the math/rand package. This change enhances code clarity and correctness. Signed-off-by: David VIEJO <[email protected]>

pfi79 · 2024-12-17T14:28:00Z

gossip/util/misc.go

+	rMutex.Lock()
+	defer rMutex.Unlock()


why is the lock/unlock call only here?

This is the panic that happens when there's a lot of concurrency:

panic: runtime error: index out of range [-1]

goroutine 3305 [running]:
math/rand.(*rngSource).Uint64(0xc014386101?)
/usr/local/go/src/math/rand/rng.go:249 +0x7b
math/rand.(*Rand).Uint64(0xc0180cf980?)
/usr/local/go/src/math/rand/rand.go:104 +0x22
github.com/hyperledger/fabric/gossip/util.RandomUInt64(...)
/gossip/util/misc.go:187
github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).newNONCE(0xc006953000)
/gossip/gossip/algo/pull.go:324 +0x25
github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).initiatePull(0xc006953000)
/gossip/gossip/algo/pull.go:179 +0x11b
github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter.func1()
/gossip/gossip/algo/pull.go:126 +0x27
created by github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter in goroutine 1
/gossip/gossip/algo/pull.go:120 +0x2d5

I suppose the problem is because we are accessing this function a lot of times, so adding a lock ensures that there are no concurrency issues

Above r is used in 2 more places without lock. Why is lock not used there?

yes, this is a bug in the math/rand package. But after the release of v3.0.0 the package used 9b28d51 was changed, this change is not in the release yet. And your stacktrace is not using v2 code.

Need proof that you actually fixed the bug suddenly it was fixed by changing the package to version v2. You need a test to prove that your fixes are correct

/usr/local/go/src/math/rand/rng.go:249 +0x7b math/rand.(*Rand).Uint64(0xc0180cf980?) /usr/local/go/src/math/rand/rand.go:104 +0x22

This is the panic that happens when there's a lot of concurrency:

panic: runtime error: index out of range [-1]

goroutine 3305 [running]: math/rand.(*rngSource).Uint64(0xc014386101?) /usr/local/go/src/math/rand/rng.go:249 +0x7b math/rand.(*Rand).Uint64(0xc0180cf980?) /usr/local/go/src/math/rand/rand.go:104 +0x22 github.com/hyperledger/fabric/gossip/util.RandomUInt64(...) /gossip/util/misc.go:187 github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).newNONCE(0xc006953000) /gossip/gossip/algo/pull.go:324 +0x25 github.com/hyperledger/fabric/gossip/gossip/algo.(*PullEngine).initiatePull(0xc006953000) /gossip/gossip/algo/pull.go:179 +0x11b github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter.func1() /gossip/gossip/algo/pull.go:126 +0x27 created by github.com/hyperledger/fabric/gossip/gossip/algo.NewPullEngineWithFilter in goroutine 1 /gossip/gossip/algo/pull.go:120 +0x2d5

I suppose the problem is because we are accessing this function a lot of times, so adding a lock ensures that there are no concurrency issues

If there is a panic stemming from gossip then I fail to see how is the chaincode related to the problem.

We shouldn't fix two different problems in the same PR.

Can you move all the non gossip related code changes to a new PR?

yeah, but you see the problem, right? gossip is invoking randomUint64, and it panicking,

you are right, these are two different problems for two different panic errors

panic calls a different code than the one you're correcting.

So @pfi79 compiling the code in main should fix the issue right about gossip right?

So @pfi79 compiling the code in main should fix the issue right about gossip right?

It's possible. After updating the package to v2 I stopped catching the panic error in tests.

yacovm · 2024-12-17T15:24:41Z

gossip/gossip/algo/pull.go

@@ -319,10 +321,28 @@ func (engine *PullEngine) OnRes(items []string, nonce uint64) {
 }

 func (engine *PullEngine) newNONCE() uint64 {
-	n := uint64(0)
+	engine.lock.Lock()


we don't need to lock at all. We only create nonces from here and we already lock there.

Just loop until engine.outgoingNONCES.Exists(n) is false, no need for attempts or anything.

Signed-off-by: David VIEJO <[email protected]>

dviejokfs · 2024-12-17T18:24:58Z

@yacovm @pfi79
I have removed the changes for the gossip issue, this PR only contains the issue about the leveldb

pfi79 · 2024-12-17T19:28:31Z

@yacovm @pfi79 I have removed the changes for the gossip issue, this PR only contains the issue about the leveldb

please update the pr description.

there is not enough evidence that your change fixes the error when working with leveldb

and please look at the unit test. after your changes they dropped exactly on leveldb.

dviejokfs · 2024-12-17T20:09:45Z

@yacovm @pfi79 I have removed the changes for the gossip issue, this PR only contains the issue about the leveldb

please update the pr description.

there is not enough evidence that your change fixes the error when working with leveldb

and please look at the unit test. after your changes they dropped exactly on leveldb.

I just updated the description, I'm going to check the unit tests

Signed-off-by: David VIEJO <[email protected]>

yacovm · 2024-12-17T23:07:25Z

Since you removed all gossip code from the PR, can't you just force push and get rid of the gossip code change commits? squash all changes together and force push so it will be more clear what's going on when this is merged, otherwise we'll merge a bunch of commits that cancel each other out.

yacovm · 2024-12-17T23:08:31Z

And I will tag @denyeart here, since the only code change in this PR is related to things he has more context about than me and pfi79.

denyeart · 2024-12-18T03:27:29Z

Thanks for the update @dviejokfs , although the PR has become confusing - 9 commits, a gossip title and bunch of gossip comments, but no gossip code, and 6 bullets in description related to chaincode and ledger, with no tests. It is unclear how everything relates. I suggest to reset - close this PR and open separate PRs for each individual improvement that can stand by itself. Each individual PR should have the smallest unit of code needed for the respective improvement, along with related unit test updates. The unit tests should demonstrate the problem being fixed (failing before the change, and passing after the change). Then the smaller individual PRs can each be judged on their own merit with clarity and focused discussion.

Note that to keep the commits clean we often use the commit amend approach. This way you don't have to go through the process of squashing and then fixing up the commit/PR description. See details at https://hyperledger-fabric.readthedocs.io/en/latest/github/github.html#updating-a-pull-request.

dviejokfs · 2024-12-18T09:14:30Z

@denyeart
Related to the unit tests, these don't change, how can we reproduce something that happens when there's a lot of concurrency? The issue is in the leveldb library:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1c pc=0x12df6f8]

goroutine 9216936 [running]:
github.com/syndtr/goleveldb/leveldb.(*dbIter).Release(0xc00c41dd40)
/vendor/github.com/syndtr/goleveldb/leveldb/db_iter.go:352 +0x118
github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/statedb/stateleveldb.(*kvScanner).Close(0x0?)
/core/ledger/kvledger/txmgmt/statedb/stateleveldb/stateleveldb.go:338 +0x1c
github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/txmgr.(*resultsItr).Close(...)
/core/ledger/kvledger/txmgmt/txmgr/query_executor.go:495

pfi79 · 2024-12-18T09:23:49Z

Related to the unit tests, these don't change, how can we reproduce something that happens when there's a lot of concurrency? The issue is in the leveldb library:

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x1c pc=0x12df6f8]

leveldb has been updated. is the error still there or not? i don't know.
your changes concern the previous version.

denyeart · 2024-12-18T15:33:53Z

@dviejokfs Agreed it may not be possible to cause the error in unit tests or integration tests, but it should at least be considered whether the problem can be induced dependably so that we know when it is resolved, even if that requires a manual test. In the extreme cases we've even sometimes temporarily updated Fabric code to make a problem more likely so that we can reproduce and ensure it is fixed. Do you have ideas around this?

dviejokfs · 2024-12-20T13:19:06Z

@denyeart @yacovm @pfi79 I just created a new PR containing a unit test and the minimal fix #5084

Fixes #5001 and gossip panic when high concurrency

e8139fc

Signed-off-by: David VIEJO <[email protected]>

dviejokfs requested a review from a team as a code owner December 17, 2024 11:56

dviejokfs added 2 commits December 17, 2024 13:22

Update

0e1b634

Signed-off-by: David VIEJO <[email protected]>

yacovm reviewed Dec 17, 2024

View reviewed changes

pfi79 reviewed Dec 17, 2024

View reviewed changes

dviejokfs added 4 commits December 17, 2024 14:00

Update based on the comments

fff8c7c

Signed-off-by: David VIEJO <[email protected]>

pfi79 reviewed Dec 17, 2024

View reviewed changes

yacovm reviewed Dec 17, 2024

View reviewed changes

Revert back the changes since the bug was fixed using math/rand/v2

4907e18

Signed-off-by: David VIEJO <[email protected]>

Fix some issues for leveldb

8222642

Signed-off-by: David VIEJO <[email protected]>

dviejokfs changed the title ~~Fixes #5001 and gossip panic when high concurrency~~ Fixes #5001 Dec 18, 2024

dviejokfs closed this Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5001 #5078

Fixes #5001 #5078

dviejokfs commented Dec 17, 2024 •

edited

Loading

yacovm Dec 17, 2024

dviejokfs Dec 17, 2024

yacovm Dec 17, 2024

dviejokfs Dec 17, 2024

yacovm Dec 17, 2024

yacovm commented Dec 17, 2024

pfi79 Dec 17, 2024

dviejokfs Dec 17, 2024

pfi79 Dec 17, 2024

dviejokfs Dec 17, 2024

pfi79 Dec 17, 2024

pfi79 Dec 17, 2024

yacovm Dec 17, 2024

dviejokfs Dec 17, 2024

dviejokfs Dec 17, 2024

pfi79 Dec 17, 2024

dviejokfs Dec 17, 2024

pfi79 Dec 17, 2024

yacovm Dec 17, 2024

dviejokfs commented Dec 17, 2024

pfi79 commented Dec 17, 2024 •

edited

Loading

dviejokfs commented Dec 17, 2024 •

edited

Loading

yacovm commented Dec 17, 2024

yacovm commented Dec 17, 2024

denyeart commented Dec 18, 2024

dviejokfs commented Dec 18, 2024

pfi79 commented Dec 18, 2024

denyeart commented Dec 18, 2024 •

edited

Loading

dviejokfs commented Dec 20, 2024

Fixes #5001 #5078

Fixes #5001 #5078

Conversation

dviejokfs commented Dec 17, 2024 • edited Loading

Type of change

Description

Additional details

Related issues

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

yacovm commented Dec 17, 2024

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

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

dviejokfs commented Dec 17, 2024

pfi79 commented Dec 17, 2024 • edited Loading

dviejokfs commented Dec 17, 2024 • edited Loading

yacovm commented Dec 17, 2024

yacovm commented Dec 17, 2024

denyeart commented Dec 18, 2024

dviejokfs commented Dec 18, 2024

pfi79 commented Dec 18, 2024

denyeart commented Dec 18, 2024 • edited Loading

dviejokfs commented Dec 20, 2024

dviejokfs commented Dec 17, 2024 •

edited

Loading

pfi79 commented Dec 17, 2024 •

edited

Loading

dviejokfs commented Dec 17, 2024 •

edited

Loading

denyeart commented Dec 18, 2024 •

edited

Loading