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.

Sign up for GitHub

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
Merged

devmode: Fix devmode networking #5182

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff4d7d4
Don't broadcast txns in devmode
Eric-Warehime Mar 8, 2023
c3e2ca6
Move the broadcast conditional
Eric-Warehime Mar 8, 2023
c432886
Remove added space
Eric-Warehime Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

if node.devMode {
return nil
}
var enc []byte
var txids []transactions.Txid
for _, tx := range txgroup {
Expand Down