Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Waiting to push fix commit until CI completes for the reapplication.

Note the postgres itests fail mostly on opening, announcing, and closing channels. This is due to the use of the batch package to batch announcement update writes, while batch doesn't handle serialization errors correctly.

Looks like there are still a couple of itests failing. Will keep working on this next week.

The error message not enough elements in RWConflictPool to record a read/write conflict tells me that postgres might not be running with enough resources to handle the itests with 8644 reapplied.

This looks relevant, re some of the errors I see in the latest CI run: https://stackoverflow.com/a/42303225

Perhaps part of the issue is with the ON CONFLICT clause in many of the queries:

• ON CONFLICT Clause with Partial Indexes: Your use of ON CONFLICT with a partial unique index and a WHERE clause may not be matching the index as expected. This can lead to unexpected behavior in conflict resolution and contribute to serialization failures.
• Conflict Resolution: The ON CONFLICT clause relies on unique indexes or constraints to detect conflicts. If the clause doesn't perfectly match an existing unique index or constraint, PostgreSQL cannot efficiently perform conflict resolution, leading to increased chances of serialization failures.
• Match ON CONFLICT Clause to Unique Index: Ensure that the ON CONFLICT clause exactly matches the unique index or constraint without additional WHERE clauses that might prevent PostgreSQL from recognizing the conflict.

Based on the SO link above, we might also be lacking some needed indexes.

With closing the channel and a couple of other tests, I'm seeing logs similar to:

2024-11-01 22:13:15.503 [ERR] GRPH builder.go:1020: unable to prune routing table: unknown postgres error: ERROR: current transaction is aborted, commands ignored until end of transaction block (SQLSTATE 25P02)
2024-11-01 22:13:15.504 [ERR] GRPH builder.go:849: unable to prune graph with closed channels: unknown postgres error: ERROR: current transaction is aborted, commands ignored until end of transaction block (SQLSTATE 25P02)

when I reproduce locally, as well as in the CI logs. I'm going to pull on that thread first...

On the test config side, also seeing these:

2024-11-02 04:28:59.361 UTC [19976] ERROR:  out of shared memory
2024-11-02 04:28:59.361 UTC [19976] HINT:  You might need to increase max_pred_locks_per_transaction.

I think the first issue above is with the code, the second is a config issue, and the other config issue in my comment above are the three major failures still happening. I think the ON CONFLICT/index audit is also a good idea to ensure we minimize serialization errors, but I'd like to solve at least the code issue with the graph pruning first.

2024-11-01 22:13:15.504 [ERR] GRPH builder.go:849: unable to prune graph with closed channels

This looks like a case where we continue when we get an error, instead of checking it for a serialization error, and returning if it is.

This looks like a case where we continue when we get an error, instead of checking it for a serialization error, and returning if it is.

Yep, looking into why that isn't caught by the panic/recover mechanism.

It was actually a lack of error checking in delChannelEdgeUnsafe() when calling updateEdgePolicyDisabledIndex(). This caused the serialization error to be ignored, and the next error is the one I pasted above. Submitting a fix commit below.

Looks better as far as the errors on closing channels. Will keep working tomorrow to eliminate the other errors.

2024-11-05 03:38:04.408 UTC [14305] ERROR: out of shared memory
2024-11-05 03:38:04.408 UTC [14305] HINT: You might need to increase max_pred_locks_per_transaction.

Hmm, so we don't have great visibility into how much memory these CI machines have. Perhaps we need to modify the connection settings to reduce the number of active connections, and also tune params like max_pred_locks_per_transaction.

@djkazic has been working on a postgres+lnd tuning/perf guide, that I think we can eventually check directly into lnd.

This is also very funky:

We do two queries to just delete: select to see if exists, then delete. Instead of just trying to delete.

Stepping back a minute: perhaps the issue is with this flawed KV abstraction we have. Perhaps we should just re-create a better hierarchical KV table from scratch. We use sqlc elsewhere so we can gain by having a unified set of light abstractions over what we want to do on the SQL layer.

Here's another instance of duplicated work in CreateBucket:

We select to see if it exists, then potentially do the insert again. Instead, we can just do an UPSERT, then use RETURNING to give us the bucket id and key we care about, so a single query.

I think the way the sequence is implemented may also be problematic: we have the sequence field directly in the table, which means table locks may need to be held. The sequence gets incremented a lot for stuff like payments, or invoice. We may be able to instead split that out into another table that can be updated independently of the main table:

I've been able to reduce (but not fully eliminate) the out of shared memory and not enough elements in RWConflictPool errors locally by changing the -N parameter of the lnd-postgres container to 200, and changing the default maxconnections value in lnd to 20. This follows from this comment about how the RWConflictPool is allocated.

I've also tried treating these errors and current transaction is aborted as serialization errors, since they generally happen when too many transactions are conflicting, and that seemed to reduce the number of test failures.

In addition, I've found one more place where we get the current transaction is aborted errors due to lack of error handling, and added error handling there.

I pushed these changes above for discussion. My next step is to try to reduce the number of conflicts based on @Roasbeef's suggestions above. I'm going on vacation for the rest of the week until next Tuesday, so will keep working on this then.

I think treating the OOM errors as serialization errors ended up being a mistake. Going to take that out and push when this run is done. In addition, I'm trying doubling the max_pred_locks_per_transaction value from the default (64->128).

When the inner function panics due to a serialization error, the lock is never released, so the retry just tries acquiring the lock when it's already held

Why does this inner function panic? Is this another instance where we aren't properly catching the error? Or is it that we actually have panics in kvdb that trigger when errors fall through normal pattern matching?

Ah ok, I see now that the serialization error handling in general is based around recovering after panics to retry a transaction:

I'm not sure why we went in that direction historically. At this kvdb emulation level, we can just pass through that error (not panic), then rely on the normal serialization error handling:

I'll be off tomorrow but I'll see if I can refactor this to avoid panics later this week. I think the biggest reason is that Get() and other functions for cursors/nested buckets don't return an error and are usually called within transactions (inside the functions that do return errors).

I can likely use a channel or mutex/error field in the tx struct to pass back errors instead of a panic in these cases, and then the deferred unlocks should be executed.

Also we can sort of rely on using the "in a failed tx" errors I've started treating as serialization errors in case something after a Get() does return an error, but it's not guaranteed.

I think I have a refactor using a mutex/error in the readWriteTx struct working correctly. Iterating through some tests locally to make sure I don't hit any snags, then pushing for a full CI run. Nope, not fully working yet (seems to have the same problem which is weird since it should have eliminated the deadlocks), but I won't get to keep going on it until Thursday. Going to push to get a full CI run anyway so I have logs to come back to.

Looks like we have a clean run bitcoind-postgres with the latest push? Nice work!

Will check out the set of core commits now. I still think we can likely re structure the the queries and KV-table, but we can save that for another time.

Re the shared memory issue, I think we can get around that by bumping up the size of the CI instance we use for these postgres tests: https://docs.github.com/en/actions/using-github-hosted-runners/using-larger-runners/running-jobs-on-larger-runners

I think I've found the deadlock. With more than one DB transaction allowed in parallel for btcwallet, we're running into a deadlock similar to the following. This example is from the UTXO sweeper tests, but it can happen in other situations as well.

In one goroutine, the UTXO sweeper calls NewAddress() on the wallet. This:

• Starts a DB transaction
• Calls (*wallet.wallet) newAddress(), which
• Calls FetchScopedKeyManager() on the waddrmgr.Manager, which
• Calls RLock() and RUnlock() on the waddrmgr.Manager mutex and returns the requested scoped manager, holding no locks
• Then it calls NextExternalAddresses() on the waddrmgr.ScopedManager, which
• Calls Lock() on the ScopedManager's mutex, defers the Unlock(), and
• Calls nextAddresses() on the ScopedManager, which
• Calls WatchOnly() on the parent Manager, which
• Calls RLock() and RUnlock() on the Manager's mutex

So while the top-level Manager's mutex is locked first, it's immediately unlocked, and then the ScopedManager lock is held and then Manager is locked/unlocked

In another goroutine, we see the sweeper call GetTransactionDetails() on the wallet, which eventually:

• Starts a DB transaction
• Within that transaction, calls (*waddrmgr.Manager) Address(), which
• Calls RLock() on the Manager, and defers the RUnlock()
• Iterates through each ScopedManager to look for a matching address, which
• Calls RLock() and then RUnlock() on the ScopedManager to check if the requested address is cached, and if not
• Calls Lock() and then Unlock() on the ScopedManager to try to load/cache the requested address from the DB

So the sequence in this case is that the Manager lock is held and the ScopedManagers are locked/unlocked.

This has previously been mitigated by the fact that each of these happens inside a database transaction, which never ran in parallel. However, with parallel DB transactions made possible by this change, the inner deadlock is exposed.

I'll submit a PR next week to btcwallet to fix this, and then clean up this PR/respond to the comments above.

I've submitted btcsuite/btcwallet#967 to fix the deadlock mentioned above.