multi: avoid sending spend nftns via goroutines. #2836

dnldd · 2021-11-29T00:27:35Z

This updates call sites sending spend journal notifications to the spend journal pruner to avoid sending notifications via goroutines. This should preserve the message ordering and avoid possibly spinning up an unbounded number of goroutines.

The spend pruner has also been refactored to batch spend prunes instead of processing single notifications individually.

davecgh

I still have more review to do, but there are already enough issues to address I figured I'd get them posted.

davecgh · 2022-03-16T04:08:06Z

blockchain/internal/spendpruner/db.go

@@ -18,9 +19,9 @@ const (
 )

 var (
-	// spendConsumerDepsBucketName is the name of the bucket used in storing
-	// spend journal consumer dependencies.
-	spendConsumerDepsBucketName = []byte("spendconsumerdeps")


What about the data that is already there in the old bucket? Perhaps there is an upcoming commit that migrates it and then removes the bucket, but I don't see that being done here.

The old bucket remains, this only changed the variable name: spendConsumerDepsBucketName -> spendConsumerDependenciesBucketName.

Right, but that's my point. The actual data in the old bucket is still there and nothing is being done with it.

the bucket is still being used: the spend pruner is initialized retrieving all the persisted dependencies from the bucket and the the entries in the bucket are updated periodically based on dependencies that can be pruned (see filterPrunableDependents). See call sites for dbUpdateSpendConsumerDependencies and dbFetchSpendConsumerDependencies.

Update: yes you're right, the data in the bucket prior to the spend heights addition will just be sitting there. Working on it.

davecgh · 2022-03-17T18:43:06Z

blockchain/indexers/spendconsumer.go

 	tipHash *chainhash.Hash
 	mtx     sync.Mutex
 }

 // NewSpendConsumer initializes a spend consumer.
-func NewSpendConsumer(id string, tipHash *chainhash.Hash, queryer ChainQueryer) *SpendConsumer {
+func NewSpendConsumer(id string, tipHash *chainhash.Hash, queryer SpendDependencyQueryer) *SpendConsumer {


This is a breaking change to the public API and therefore can't be done without a major module bump to blockchain.

I was a little bit unsure about if this case could sneak through since the new interface is a strict subset of previous interface and thus anything that satisfied the old one will still satisfy the new one, however, after discussing it in the dev channel, it is indeed a breaking change regardless.

As @jrick noted, a caller might be creating a closure that would break.

Translating it to a concrete example for this change, imagine if a caller had the following code:

var x func(string, *chainhash.Hash, indexers.ChainQueryer) = indexers.NewSpendConsumer

This change would cause the caller's code to fail to compile, which is expressly forbidden without a major module bump.

Noted, will wait for #2903 to be merged and rebase.

blockchain/internal/spendpruner/db.go

blockchain/internal/spendpruner/db_test.go

davecgh · 2022-03-17T19:17:40Z

blockchain/internal/spendpruner/db.go

+
+	// Persist all spend height map entries.
+	for blockHash, height := range spendHeights {
+		var b [8]byte


Any reason for using 8 when you're encoding a uint32 which is 4 bytes?

Nope, should be 4 bytes there. Updating.

davecgh · 2022-03-17T19:20:11Z

blockchain/internal/spendpruner/db.go

+	}
+
+	// Persist all spend height map entries.
+	for blockHash, height := range spendHeights {


All of the code in this commit is chalk full of incorrect usage. The range statement overwrites the variables with each iteration and you're slicing them directly and passing them on to db code, so with any type of async behavior, it will be a race and overwrite things incorrectly.

package main import ( "encoding/binary" "fmt" "time" ) func put(k []byte, v []byte) { go func(k, v []byte) { time.Sleep(time.Second) fmt.Printf("k: %x, v: %x\n", k, v) }(k, v) } func main() { foo := map[[32]byte]uint32{ [32]byte{0x00}: 0, [32]byte{0x01}: 1, [32]byte{0x02}: 2, } for hash, height := range foo { var b [4]byte binary.LittleEndian.PutUint32(b[:], height) put(hash[:], b[:]) } time.Sleep(time.Second * 2) }

Output:

k: 0200000000000000000000000000000000000000000000000000000000000000, v: 00000000 k: 0200000000000000000000000000000000000000000000000000000000000000, v: 01000000 k: 0200000000000000000000000000000000000000000000000000000000000000, v: 02000000

Noted, thanks. Updating.

davecgh · 2022-03-17T19:22:08Z

blockchain/internal/spendpruner/db.go

+
+	// Persist all spend height map entries.
+	for _, blockHash := range keys {
+		err := heightsBucket.Delete(blockHash[:])


Incorrect as per previous comments.

blockchain/internal/spendpruner/db.go

dnldd · 2022-03-17T20:22:47Z

Thanks for the review 👍🏾, will start addressing the issues soon.

davecgh · 2022-06-24T05:18:59Z

Needs a rebase please.

This refactors the spend consumer by updating the queryer interface it uses. The chain queryer interface has been replaced by a lighter one: SpendDependencyQueryer.

This adds a spend journal height bucket for tracking the heights of spend journal entries needed by consumers. database related helpers have also been added.

This revers shorthands of the word dependency in function/mthod names and comments to avoid confusion.

This adds a spend consumer to track spend dependencies needed for invalidating and reconsidering blocks.

This updates call sites sending spend journal notifications to the spend journal pruner to avoid sending notifications via goroutines. This should preserve the message ordering and avoid possibly spinning up an unbounded number of goroutines. The spend pruner has also been refactored to batch spend prunes instead of processing single notifications individually.

This updates the chainSetup function to return a startup function. Call sites have been updated accordingly.

This adds generateDependencySpendHeights to generate the associated spend heights for existing spend dependencies before the introduction of spend heights. Associated tests have been added.

davecgh · 2022-06-29T03:32:06Z

I see this was updated, but it doesn't look like it was fully rebased to play nicely with #2961 and hence the latest master. It's touching a lot of spend pruner code that is no longer in master.

I'll try to review the updates around it in the mean time, but it does need to be rebased to account for the removal of the spend pruner.

dnldd · 2022-06-29T13:23:19Z

Noted, will update soon.

dnldd · 2022-06-29T22:39:02Z

After a closer look the changes made here were focused on the spend pruner, I initially thought the startup func additions made to chainSetup could be kept but the only thing that was using it was the spend pruner. Closing this PR as a result.

dnldd force-pushed the spend_pruner_improvments branch from 0785236 to 3cd3187 Compare December 8, 2021 23:12

dnldd force-pushed the spend_pruner_improvments branch 3 times, most recently from 5dc3b56 to 962c417 Compare February 1, 2022 10:17

dnldd marked this pull request as ready for review February 1, 2022 10:21

davecgh requested changes Mar 17, 2022

View reviewed changes

davecgh mentioned this pull request Mar 17, 2022

indexers: fix indexer wait for sync. #2871

Merged

dnldd force-pushed the spend_pruner_improvments branch 3 times, most recently from 684dcb4 to 720304d Compare March 24, 2022 01:10

dnldd added 7 commits June 26, 2022 20:39

indexers: refactor spend consumer.

3a7b9b4

This refactors the spend consumer by updating the queryer interface it uses. The chain queryer interface has been replaced by a lighter one: SpendDependencyQueryer.

spendpruner: track spend journal heights.

c07e50c

This adds a spend journal height bucket for tracking the heights of spend journal entries needed by consumers. database related helpers have also been added.

spendpruner: avoid shortenening 'dependency'.

9b94b24

This revers shorthands of the word dependency in function/mthod names and comments to avoid confusion.

multi: add invalidate/reconsider spend consumer.

73f38f9

This adds a spend consumer to track spend dependencies needed for invalidating and reconsidering blocks.

multi: return startup function for chaingen.

3b719d5

This updates the chainSetup function to return a startup function. Call sites have been updated accordingly.

spendpruner: add generateDependencySpendHeights.

66bde17

This adds generateDependencySpendHeights to generate the associated spend heights for existing spend dependencies before the introduction of spend heights. Associated tests have been added.

dnldd force-pushed the spend_pruner_improvments branch from 720304d to 66bde17 Compare June 26, 2022 21:18

dnldd closed this Jun 29, 2022

dnldd deleted the spend_pruner_improvments branch June 30, 2022 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: avoid sending spend nftns via goroutines. #2836

multi: avoid sending spend nftns via goroutines. #2836

dnldd commented Nov 29, 2021 •

edited

Loading

davecgh left a comment

davecgh Mar 16, 2022

dnldd Mar 22, 2022

davecgh Mar 22, 2022

dnldd Mar 22, 2022 •

edited

Loading

davecgh Mar 17, 2022

dnldd Mar 22, 2022

davecgh Mar 17, 2022 •

edited

Loading

dnldd Mar 22, 2022

davecgh Mar 17, 2022

dnldd Mar 22, 2022

davecgh Mar 17, 2022

dnldd commented Mar 17, 2022

davecgh commented Jun 24, 2022

davecgh commented Jun 29, 2022

dnldd commented Jun 29, 2022

dnldd commented Jun 29, 2022

multi: avoid sending spend nftns via goroutines. #2836

multi: avoid sending spend nftns via goroutines. #2836

Conversation

dnldd commented Nov 29, 2021 • edited Loading

davecgh 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

dnldd Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Mar 17, 2022 • edited Loading

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

dnldd commented Mar 17, 2022

davecgh commented Jun 24, 2022

davecgh commented Jun 29, 2022

dnldd commented Jun 29, 2022

dnldd commented Jun 29, 2022

dnldd commented Nov 29, 2021 •

edited

Loading

dnldd Mar 22, 2022 •

edited

Loading

davecgh Mar 17, 2022 •

edited

Loading