Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors #815

bartekn · 2019-01-16T18:31:09Z

This commit fixes two issues in txsub package. First, issue was a
deadlock in Tick() function:

sys.tickMutex.Lock()
if sys.tickInProgress {
  logger.Debug("ticking in progress")
  return
}
sys.tickInProgress = true
sys.tickMutex.Unlock()

When two Tick() methods were called simultanously one of them could
Lock() the mutex without calling Unlock() properly. This could
basically break the txsub system as sequence number could not be
updated.

The second issue was a problem with App initialization.
App.networkPassphrase was set to test network passphrase by default.
If txsub system was initialized before updating the network passphrase
using connected stellar-core, the system would not be able to
correctly find the transaction result in a database as calculated hash
was different than the actual hash of the transaction.

Close #305.

This commit fixes to issues in `txsub` package. First, issue was a deadlock in `Tick()` function: ```go sys.tickMutex.Lock() if sys.tickInProgress { logger.Debug("ticking in progress") return } sys.tickInProgress = true sys.tickMutex.Unlock() ``` When two `Tick()` methods were called simultanously one of them could `Lock()` the mutex without calling `Unlock()` it properly. This could basically break the `txsub` system as sequence number could not be updated. The second issue was a problem with `App` initialization. `App.networkPassphrase` was set to test network passphrase by default. If `txsub` system was initialized before updating the network passphrase using connected `stellar-core`, the system would not be able to correctly find the transaction result in a database as calculated hash was different than the actual hash of the transaction.

tomquisel

This looks like a great fix to me, Bartek! I had a few questions, I know this is a rush fix, so feel free to ignore suggestions in the interest of getting it out the door.

tomquisel · 2019-01-17T00:02:07Z

services/horizon/internal/app.go

+		log.Error("Network passhprase of stellar-core does not match Horizon configuration. Exiting...")
+		os.Exit(1)
+	}
+
 	a.coreVersion = resp.Info.Build
 	a.networkPassphrase = resp.Info.Network


This line can be removed now, right?

tomquisel · 2019-01-17T00:02:30Z

services/horizon/internal/app.go

+	// Check if NetworkPassphrase is different, if so exit Horizon as it can break the
+	// state of the application.
+	if resp.Info.Network != a.networkPassphrase {
+		log.Error("Network passhprase of stellar-core does not match Horizon configuration. Exiting...")


change passhprase to passphrase, and maybe improve error message to print the two passphrase values.

tomquisel · 2019-01-17T00:06:10Z

services/horizon/internal/app.go

@@ -64,7 +64,7 @@ func NewApp(config Config) (*App, error) {

 	result := &App{config: config}
 	result.horizonVersion = app.Version()
-	result.networkPassphrase = build.TestNetwork.Passphrase
+	result.networkPassphrase = config.NetworkPassphrase


Could we remove App.networkPassphrase and just use App.Config.NetworkPassphrase as the single source of truth?

tomquisel · 2019-01-17T00:43:52Z

services/horizon/internal/txsub/system.go

@@ -178,7 +187,8 @@ func (sys *System) Tick(ctx context.Context) {
 	// Make sure Tick is not run concurrently
 	sys.tickMutex.Lock()


I might accomplish this locking with two functions:

SetTickInProgress:

sys.tickMutex.Lock() defer sys.tickMutex.Unlock() if sys.tickInProgress { logger.Debug("ticking in progress") return false } sys.tickInProgress = true return true

UnsetTickInProgress:

sys.tickMutex.Lock() defer sys.tickMutex.Unlock() sys.tickInProgress = false

and then call those from Tick (which checks the results of SetTickInProgress and exits if false, and calls UnsetTickInProgress() in defer.

I think that'd make this a little safer from this kind of bug in the future.

Good point. The ultimate solution is to remove global ticking service completely (#613). It caused a lot of concurrency problems in the past (including this one) and can be implemented much easier using infinite loop.

tomquisel · 2019-01-17T00:51:10Z

services/horizon/main.go

+	if viper.GetString("network-passphrase") == "" {
+		stdLog.Fatal("Invalid config: network-passphrase is blank.  Please specify --network-passphrase on the command line or set the NETWORK_PASSPHRASE environment variable.")
+	}
+


Hrm, is it a breaking change to make this required? It seems like it might be, and it'd be better to leave network-passphrase optional + default to testnet as we do currently.

Good point, set default value to testnet passphrase but in the next minor we need to make this empty by default. Ex. if you run ingestion node without web server and when stellar-core is not running it should be explicitly set by Horizon admin (#817).

oryband · 2019-01-17T13:31:42Z

I've tested this branch in an environment where I can reproduce the issue and the problem is solved.

bartekn · 2019-01-17T13:50:57Z

@oryband thanks for checking this. Would you mind checking this again in your environment (I pushed one last commit with updates).
@tomquisel all good points, fixed and added 2 comments.

oryband · 2019-01-17T15:04:22Z

@bartekn I tested again, and it works. Thanks for sorting this issue out! 🏆

oryband · 2019-01-17T15:05:04Z

@bartekn can you release a new stable version that includes this branch?

bartekn · 2019-01-17T15:05:23Z

@oryband today.

oryband · 2019-01-17T15:05:38Z

WHOOHOO.

ire-and-curses

LGTM!

howardtw

Minor suggestion: maybe it's worth breaking PRs fixing two issues into two smaller ones in the future? e.g. one for the tick, one for the passphrase

howardtw · 2019-01-18T00:46:47Z

services/horizon/internal/txsub/system.go

@@ -170,25 +179,40 @@ func (sys *System) submitOnce(ctx context.Context, env string) SubmissionResult
 	return sr
 }

+// setTickInProgress sets `tickInProgress` to `true` if it's not


Looks like we set tickInProgress to true only if it is false?

Good catch! #824

howardtw · 2019-01-18T00:50:56Z

services/horizon/internal/txsub/system.go

@@ -73,12 +74,20 @@ func (sys *System) Submit(ctx context.Context, env string) (result <-chan Result
 	// check the configured result provider for an existing result
 	r := sys.Results.ResultByHash(ctx, info.Hash)

-	if r.Err != ErrNoResults {
+	if r.Err == nil {


Nothing wrong with the logic here but what is the reason that we have an Err field in r as oppose to have ResultByHash return r and err directly?

It's also used as an object returned by channel.

will resolve conflicts in following commits * tag 'horizon-v0.15.4': Horizon 0.15.4 CHANGELOG (stellar#818) Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors (stellar#815) Add information about test scenarios to docs (stellar#813) trades endpoint does not have resolution (stellar#733) fix links and update orderbook field names (stellar#750) Doc: flesh out docs for developers (stellar#804) Fix graceful shutdown (stellar#803) Log client name and version (stellar#727) Use go vet in CI (stellar#611) Fix panic when missing an argument for horizon db backfill (stellar#806)

bartekn added the horizon label Jan 16, 2019

bartekn added this to the Horizon patch releases milestone Jan 16, 2019

bartekn requested review from tomquisel, ire-and-curses and howardtw January 16, 2019 18:31

Fix tests

a3a0c80

tomquisel reviewed Jan 17, 2019

View reviewed changes

bartekn mentioned this pull request Jan 17, 2019

Horizon timeout after transaction is added to ledger #305

Closed

oryband mentioned this pull request Jan 17, 2019

Disallow submitting transactions if stellar-core is not synced #801

Closed

Review comments

59446eb

Fix test

8a0a886

ire-and-curses approved these changes Jan 17, 2019

View reviewed changes

tomquisel approved these changes Jan 17, 2019

View reviewed changes

bartekn merged commit 1a536e3 into master Jan 17, 2019

bartekn deleted the txsub-improvements branch January 17, 2019 18:22

howardtw reviewed Jan 18, 2019

View reviewed changes

bartekn mentioned this pull request Jul 9, 2019

SEP-10 should define a network passphrase used when signing challenge transaction stellar/stellar-protocol#339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors #815

Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors #815

bartekn commented Jan 16, 2019 •

edited

Loading

tomquisel left a comment

tomquisel Jan 17, 2019

tomquisel Jan 17, 2019

tomquisel Jan 17, 2019

tomquisel Jan 17, 2019

bartekn Jan 17, 2019

tomquisel Jan 17, 2019

bartekn Jan 17, 2019

oryband commented Jan 17, 2019

bartekn commented Jan 17, 2019

oryband commented Jan 17, 2019 •

edited

Loading

oryband commented Jan 17, 2019 •

edited

Loading

bartekn commented Jan 17, 2019

oryband commented Jan 17, 2019

ire-and-curses left a comment

howardtw left a comment •

edited

Loading

howardtw Jan 18, 2019

bartekn Jan 22, 2019

howardtw Jan 18, 2019

bartekn Jan 22, 2019

		@@ -178,7 +187,8 @@ func (sys *System) Tick(ctx context.Context) {
		// Make sure Tick is not run concurrently
		sys.tickMutex.Lock()

Fix deadlock in txsub.System.Tick() and tx_bad_seq errors #815

Fix deadlock in txsub.System.Tick() and tx_bad_seq errors #815

Conversation

bartekn commented Jan 16, 2019 • edited Loading

tomquisel 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

Choose a reason for hiding this comment

oryband commented Jan 17, 2019

bartekn commented Jan 17, 2019

oryband commented Jan 17, 2019 • edited Loading

oryband commented Jan 17, 2019 • edited Loading

bartekn commented Jan 17, 2019

oryband commented Jan 17, 2019

ire-and-curses left a comment

Choose a reason for hiding this comment

howardtw left a comment • 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

Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors #815

Fix deadlock in `txsub.System.Tick()` and `tx_bad_seq` errors #815

bartekn commented Jan 16, 2019 •

edited

Loading

oryband commented Jan 17, 2019 •

edited

Loading

oryband commented Jan 17, 2019 •

edited

Loading

howardtw left a comment •

edited

Loading