Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devmode: Fix devmode networking #5182

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

Eric-Warehime
Copy link
Contributor

@Eric-Warehime Eric-Warehime commented Mar 8, 2023

Summary

Skip network broadcast of transactions if the node is in devMode.

This fixes a regression that was introduced by 51d5925#diff-375d57e386f20eaa5f09f02bb9d28bfc48ac3dca18d0325f59492208219e5618L188

Test Plan

Sdk tests (formerly failing with broadcast queue full see https://app.circleci.com/pipelines/github/algorand/go-algorand-sdk/771/workflows/d69fed8b-61bd-451b-9315-4f1900af7fdb/jobs/2124 ) all pass locally w/ this commit.

@Eric-Warehime
Copy link
Contributor Author

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #5182 (c432886) into master (4bbedc0) will decrease coverage by 1.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5182      +/-   ##
==========================================
- Coverage   53.51%   52.50%   -1.01%     
==========================================
  Files         439      439              
  Lines       54985    54987       +2     
==========================================
- Hits        29423    28870     -553     
- Misses      23271    23783     +512     
- Partials     2291     2334      +43     
Impacted Files Coverage Δ
node/node.go 4.10% <0.00%> (-0.02%) ⬇️
data/transactions/error.go 0.00% <0.00%> (-50.00%) ⬇️
logging/cyclicWriter.go 0.00% <0.00%> (-46.47%) ⬇️
tools/network/dnssec/client.go 44.11% <0.00%> (-38.24%) ⬇️
tools/network/dnssec/util.go 77.77% <0.00%> (-22.23%) ⬇️
cmd/tealdbg/cdtdbg.go 63.52% <0.00%> (-18.83%) ⬇️
cmd/algocfg/getCommand.go 12.00% <0.00%> (-16.00%) ⬇️
data/txHandler.go 57.05% <0.00%> (-15.02%) ⬇️
network/netidentity.go 85.04% <0.00%> (-14.96%) ⬇️
tools/network/dnssec/trustedzone.go 86.07% <0.00%> (-12.66%) ⬇️
... and 50 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Eric-Warehime Eric-Warehime requested a review from shiqizng March 8, 2023 18:27
@@ -535,7 +535,10 @@ func (node *AlgorandFullNode) broadcastSignedTxGroup(txgroup []transactions.Sign
if err != nil {
logging.Base().Infof("unable to pin transaction: %v", err)
}

// DevMode nodes do not broadcast txns to the network
Copy link
Contributor

@tzaffi tzaffi Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could cause a bit of confusion, because the method name is broadcastSignedTxGroup. Since this method is only referred to in 2 places, there is a way to refactor without too much pain. I.e., you can extract the non-broadcasting logic into its own function (say it's called foo()), then have BroadcastSignedTxGroup call foo() instead of broadcastSignedTxGroup for dev mode - that is.
But again, this is just a nit so feel free to ignore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and we're still stuck with public BroadcastSignedTxGroup regardless of the above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like the current implementation better because if I refactor (as below) it would duplicate the conditional and as you stated we're still not broadcasting in some situations from the BroadcastSignedTxGroup method.

func validateAndPin(txgroup []transactions.SignedTxn) (err error) {
    ...
}

func broadcastSignedTxGroup(txgroup []transactions.SignedTxn) (err error) {
    ....
}

func BroadcastSignedTxGroup(txgroup []transactions.SignedTxn) (err error) {
    if node.devMode {
	node.mu.Lock()
	defer func() {
		// if we added the transaction successfully to the transaction pool, then
		// attempt to generate a block and write it to the ledger.
		if err == nil {
			err = node.writeDevmodeBlock()
		}
		node.mu.Unlock()
	}()
    }
    if err = validateAndPin(txgroup); err != nil {
        return err
    }
    if !node.devMode {
        return broadcastSignedTxGroup(txgroup)
    }
}

func BroadcastInternalSignedTxGroup(txgroup []transactions.SignedTxn) (err error) {
    if err = validateAndPin(txgroup); err != nil {
        return err
    }
    if !node.devMode {
        return broadcastSignedTxGroup(txgroup)
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's simpler to just rename private broadcastSignedTxGroup to be verifyAndNonDevBroadcast (if you can find a better name pls). Regardless, I'm going to approve since this is all just a nit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty much fine keeping this as a conditional...In general I agree that it would be better to refactor things so that functionality is kept contained in methods relevant to each operational mode. However, that's not how dev mode was implemented--it's just a series of these small conditionals that alter the regular behavior.

In this case, I don't think we're particularly benefitting from splitting this method into 3-4 separate ones, especially since it still requires similar conditionals.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could take or leave Zeph's suggestion, I'll leave it to @Eric-Warehime . While it's peculiar for BroadCastSignedTxGroup to succeed without broadcasted anything, we know that DevMode is enabled so it doesn't need to. Somehow that doesn't seem so bad.

@cce
Copy link
Contributor

cce commented Mar 9, 2023

Since this is a side effect of removing the if devMode { DisableNetworking = true} change from https://github.com/algorand/go-algorand/pull/5157/files/95fce85d678ecd36244c2c4709d2e3365f081451#r1118944841 so why not revert it? Or set DisableNetworking = true in the config for these types of tests? It comes down to whether you want dev mode to have networking disabled by default, or if you want it to support the dev-and-follow-node configuration by default which seems like a very special case. Beyond this there there are likely other node behavior differences from having networking enabled for dev mode that users may not expect as well.

@cce
Copy link
Contributor

cce commented Mar 9, 2023

For example you could leave devmode with networking disabled by default as it was before, but add a new option for running dev mode + networking to support testing the dev+follow node topology, which I'd argue is a much rarer use case that does not justify the breaking change in #5157.

@Eric-Warehime
Copy link
Contributor Author

Eric-Warehime commented Mar 9, 2023

For example you could leave devmode with networking disabled by default as it was before, but add a new option for running dev mode + networking to support testing the dev+follow node topology, which I'd argue is a much rarer use case that does not justify the breaking change in #5157.

Let me think about it...I suspect that disabling broadcasting as I've done here would fix single node devmode networks and break multi-node setups. So I agree we need to probably just disable networking in single node networks in general right?

Edit: Disabling broadcasting isn't something which would affect multi-node setups since devmode networks won't operate in the traditional consensus setup.

@Eric-Warehime
Copy link
Contributor Author

@cce I've discussed a bit more w/ others and I'm not sure what adding an optional enable/disable networking to devmode gets us. Because of the follower node, we now need networking to work w/ devmode nodes so this fix still needs to happen regardless.

In terms of optionally disabling networking in devmode, running it w/ these changes should provide the same experience as running a devmode node w/o networking. While I agree that enabling networking could have more undiscovered bugs than just this one (though I don't know of any), they're all things we would need to fix in the end anyways.

@algorandskiy
Copy link
Contributor

algorandskiy commented Mar 9, 2023

I do not like devmode switch to be scattered across the code. But what are the options?..

@winder
Copy link
Contributor

winder commented Mar 10, 2023

@algorandskiy not sure what other option there is. The functionality needs to be toggled somehow. Short of creating a new node type (i.e. follower node) this seems to be the way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants