From dac0ad75571c53de2a89395ee41b242f5ee15f09 Mon Sep 17 00:00:00 2001 From: Derek Leung Date: Thu, 9 Apr 2020 13:06:45 -0400 Subject: [PATCH 1/4] Bugfix. (#959) --- config/consensus.go | 16 +- data/bookkeeping/block.go | 2 +- ledger/ledger_test.go | 80 ++++++++++ ledger/txtail.go | 11 +- protocol/consensus.go | 7 +- .../features/transactions/lease_test.go | 139 +++++++++++++++++- .../upgrades/send_receive_upgrade_test.go | 16 ++ .../nettemplates/TwoNodes50EachV22.json | 35 +++++ .../TwoNodes50EachV22Upgrade.json | 35 +++++ 9 files changed, 332 insertions(+), 9 deletions(-) create mode 100644 test/testdata/nettemplates/TwoNodes50EachV22.json create mode 100644 test/testdata/nettemplates/TwoNodes50EachV22Upgrade.json diff --git a/config/consensus.go b/config/consensus.go index 1dba9b75b6..9c9f25fb19 100644 --- a/config/consensus.go +++ b/config/consensus.go @@ -192,7 +192,12 @@ type ConsensusParams struct { MaxTxGroupSize int // support for transaction leases + // note: if FixTransactionLeases is not set, the transaction + // leases supported are faulty; specifically, they do not + // enforce exclusion correctly when the FirstValid of + // transactions do not match. SupportTransactionLeases bool + FixTransactionLeases bool // 0 for no support, otherwise highest version supported LogicSigVersion uint64 @@ -551,9 +556,18 @@ func initConsensusProtocols() { // v21 can be upgraded to v22. v21.ApprovedUpgrades[protocol.ConsensusV22] = 0 + // v23 is an upgrade which fixes the behavior of leases so that + // it conforms with the intended spec. + v23 := v22 + v23.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{} + v23.FixTransactionLeases = true + Consensus[protocol.ConsensusV23] = v23 + // v22 can be upgraded to v23. + v22.ApprovedUpgrades[protocol.ConsensusV23] = 10000 + // ConsensusFuture is used to test features that are implemented // but not yet released in a production protocol version. - vFuture := v22 + vFuture := v23 vFuture.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{} Consensus[protocol.ConsensusFuture] = vFuture } diff --git a/data/bookkeeping/block.go b/data/bookkeeping/block.go index a5010ed208..54426066b6 100644 --- a/data/bookkeeping/block.go +++ b/data/bookkeeping/block.go @@ -316,7 +316,7 @@ func (s UpgradeState) applyUpgradeVote(r basics.Round, vote UpgradeVote) (res Up upgradeDelay := uint64(vote.UpgradeDelay) if upgradeDelay > params.MaxUpgradeWaitRounds || upgradeDelay < params.MinUpgradeWaitRounds { - err = fmt.Errorf("applyUpgradeVote: proposed upgrade wait rounds %d out of permissible range", upgradeDelay) + err = fmt.Errorf("applyUpgradeVote: proposed upgrade wait rounds %d out of permissible range [%d, %d]", upgradeDelay, params.MinUpgradeWaitRounds, params.MaxUpgradeWaitRounds) return } diff --git a/ledger/ledger_test.go b/ledger/ledger_test.go index e06bb74008..6b2a0bbee7 100644 --- a/ledger/ledger_test.go +++ b/ledger/ledger_test.go @@ -777,3 +777,83 @@ func TestLedgerSingleTxApplyDataV18(t *testing.T) { func TestLedgerSingleTxApplyDataFuture(t *testing.T) { testLedgerSingleTxApplyData(t, protocol.ConsensusFuture) } + +func TestLedgerRegressionFaultyLeaseFirstValidCheckOld(t *testing.T) { + testLedgerRegressionFaultyLeaseFirstValidCheck2f3880f7(t, protocol.ConsensusV22) +} + +func TestLedgerRegressionFaultyLeaseFirstValidCheckV23(t *testing.T) { + testLedgerRegressionFaultyLeaseFirstValidCheck2f3880f7(t, protocol.ConsensusV23) +} + +func TestLedgerRegressionFaultyLeaseFirstValidCheck(t *testing.T) { + testLedgerRegressionFaultyLeaseFirstValidCheck2f3880f7(t, protocol.ConsensusCurrentVersion) +} + +func TestLedgerRegressionFaultyLeaseFirstValidCheckFuture(t *testing.T) { + testLedgerRegressionFaultyLeaseFirstValidCheck2f3880f7(t, protocol.ConsensusFuture) +} + +func testLedgerRegressionFaultyLeaseFirstValidCheck2f3880f7(t *testing.T, version protocol.ConsensusVersion) { + a := require.New(t) + + backlogPool := execpool.MakeBacklog(nil, 0, execpool.LowPriority, nil) + defer backlogPool.Shutdown() + + genesisInitState, initSecrets := testGenerateInitState(t, version) + const inMem = true + const archival = true + log := logging.TestingLog(t) + l, err := OpenLedger(log, t.Name(), inMem, genesisInitState, archival) + a.NoError(err, "could not open ledger") + defer l.Close() + + proto := config.Consensus[version] + poolAddr := testPoolAddr + sinkAddr := testSinkAddr + + initAccounts := genesisInitState.Accounts + var addrList []basics.Address + for addr := range initAccounts { + if addr != poolAddr && addr != sinkAddr { + addrList = append(addrList, addr) + } + } + + correctTxHeader := transactions.Header{ + Sender: addrList[0], + Fee: basics.MicroAlgos{Raw: proto.MinTxnFee * 2}, + FirstValid: l.Latest() + 1, + LastValid: l.Latest() + 10, + GenesisID: t.Name(), + GenesisHash: crypto.Hash([]byte(t.Name())), + } + + correctPayFields := transactions.PaymentTxnFields{ + Receiver: addrList[1], + Amount: basics.MicroAlgos{Raw: initAccounts[addrList[0]].MicroAlgos.Raw / 10}, + } + + correctPay := transactions.Transaction{ + Type: protocol.PaymentTx, + Header: correctTxHeader, + PaymentTxnFields: correctPayFields, + } + + var ad transactions.ApplyData + + correctPayLease := correctPay + correctPayLease.Sender = addrList[3] + correctPayLease.Lease[0] = 1 + + a.NoError(l.appendUnvalidatedTx(t, initAccounts, initSecrets, correctPayLease, ad), "could not add initial payment transaction") + + correctPayLease.FirstValid = l.Latest() + 1 + correctPayLease.LastValid = l.Latest() + 10 + + if proto.FixTransactionLeases { + a.Error(l.appendUnvalidatedTx(t, initAccounts, initSecrets, correctPayLease, ad), "added payment transaction with overlapping lease") + } else { + a.NoError(l.appendUnvalidatedTx(t, initAccounts, initSecrets, correctPayLease, ad), "should allow leasing payment transaction with newer FirstValid") + } +} diff --git a/ledger/txtail.go b/ledger/txtail.go index bac5eb13ca..f51fb64f4b 100644 --- a/ledger/txtail.go +++ b/ledger/txtail.go @@ -36,7 +36,7 @@ type txTail struct { lastValid map[basics.Round]map[transactions.Txid]struct{} // map tx.LastValid -> tx confirmed set - // duplicate detection queries with LastValid not before + // duplicate detection queries with LastValid before // lowWaterMark are not guaranteed to succeed lowWaterMark basics.Round // the last round known to be committed to disk } @@ -128,7 +128,14 @@ func (t *txTail) isDup(proto config.ConsensusParams, current basics.Round, first } if proto.SupportTransactionLeases && (txl.lease != [32]byte{}) { - for rnd := firstValid; rnd <= lastValid; rnd++ { + firstChecked := firstValid + lastChecked := lastValid + if proto.FixTransactionLeases { + firstChecked = current.SubSaturate(basics.Round(proto.MaxTxnLife)) + lastChecked = current + } + + for rnd := firstChecked; rnd <= lastChecked; rnd++ { expires, ok := t.recent[rnd].txleases[txl] if ok && current <= expires { return true, nil diff --git a/protocol/consensus.go b/protocol/consensus.go index 9fd872e563..86928db447 100644 --- a/protocol/consensus.go +++ b/protocol/consensus.go @@ -123,6 +123,11 @@ const ConsensusV22 = ConsensusVersion( "https://github.com/algorandfoundation/specs/tree/57016b942f6d97e6d4c0688b373bb0a2fc85a1a2", ) +// ConsensusV23 fixes lease behavior. +const ConsensusV23 = ConsensusVersion( + "https://github.com/algorandfoundation/specs/tree/e5f565421d720c6f75cdd186f7098495caf9101f", +) + // ConsensusFuture is a protocol that should not appear in any production // network, but is used to test features before they are released. const ConsensusFuture = ConsensusVersion( @@ -135,7 +140,7 @@ const ConsensusFuture = ConsensusVersion( // ConsensusCurrentVersion is the latest version and should be used // when a specific version is not provided. -const ConsensusCurrentVersion = ConsensusV22 +const ConsensusCurrentVersion = ConsensusV23 // Error is used to indicate that an unsupported protocol has been detected. type Error ConsensusVersion diff --git a/test/e2e-go/features/transactions/lease_test.go b/test/e2e-go/features/transactions/lease_test.go index cd2d0698e2..29366f7680 100644 --- a/test/e2e-go/features/transactions/lease_test.go +++ b/test/e2e-go/features/transactions/lease_test.go @@ -31,7 +31,7 @@ func TestLeaseTransactionsSameSender(t *testing.T) { a := require.New(t) var fixture fixtures.RestClientFixture - fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachFuture.json")) + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50Each.json")) defer fixture.Shutdown() client := fixture.LibGoalClient @@ -85,12 +85,143 @@ func TestLeaseTransactionsSameSender(t *testing.T) { a.Equal(bal2, uint64(0)) } +func TestLeaseRegressionFaultyFirstValidCheckOld_2f3880f7(t *testing.T) { + t.Parallel() + a := require.New(t) + + var fixture fixtures.RestClientFixture + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachV22.json")) + defer fixture.Shutdown() + + client := fixture.LibGoalClient + accountList, err := fixture.GetWalletsSortedByBalance() + a.NoError(err) + account0 := accountList[0].Address + wh, err := client.GetUnencryptedWalletHandle() + a.NoError(err) + + account1, err := client.GenerateAddress(wh) + a.NoError(err) + + account2, err := client.GenerateAddress(wh) + a.NoError(err) + + lease := [32]byte{1, 2, 3, 4} + + // construct transactions for sending money to account1 and account2 + // from same sender with identical lease + tx1, err := client.ConstructPayment(account0, account1, 0, 1000000, nil, "", lease, 0, 0) + a.NoError(err) + + stx1, err := client.SignTransactionWithWallet(wh, nil, tx1) + a.NoError(err) + + // submitting the first transaction should succeed + _, err = client.BroadcastTransaction(stx1) + a.NoError(err) + + // wait for the txids and check balance + txids := make(map[string]string) + txids[stx1.Txn.ID().String()] = account0 + + _, curRound := fixture.GetBalanceAndRound(account0) + confirmed := fixture.WaitForAllTxnsToConfirm(curRound+5, txids) + a.True(confirmed, "lease txn confirmed") + + bal1, _ := fixture.GetBalanceAndRound(account1) + bal2, _ := fixture.GetBalanceAndRound(account2) + a.Equal(bal1, uint64(1000000)) + a.Equal(bal2, uint64(0)) + + tx2, err := client.ConstructPayment(account0, account2, 0, 2000000, nil, "", lease, 0, 0) + a.NoError(err) + + stx2, err := client.SignTransactionWithWallet(wh, nil, tx2) + a.NoError(err) + + // submitting the second transaction should succeed + _, err = client.BroadcastTransaction(stx2) + a.NoError(err) + + // wait for the txids and check balance + txids = make(map[string]string) + txids[stx2.Txn.ID().String()] = account0 + + _, curRound = fixture.GetBalanceAndRound(account0) + confirmed = fixture.WaitForAllTxnsToConfirm(curRound+5, txids) + a.True(confirmed, "lease txn confirmed") + + bal1, _ = fixture.GetBalanceAndRound(account1) + bal2, _ = fixture.GetBalanceAndRound(account2) + a.Equal(bal1, uint64(1000000)) + a.Equal(bal2, uint64(2000000)) +} + +func TestLeaseRegressionFaultyFirstValidCheckNew_2f3880f7(t *testing.T) { + t.Parallel() + a := require.New(t) + + var fixture fixtures.RestClientFixture + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50Each.json")) + defer fixture.Shutdown() + + client := fixture.LibGoalClient + accountList, err := fixture.GetWalletsSortedByBalance() + a.NoError(err) + account0 := accountList[0].Address + wh, err := client.GetUnencryptedWalletHandle() + a.NoError(err) + + account1, err := client.GenerateAddress(wh) + a.NoError(err) + + account2, err := client.GenerateAddress(wh) + a.NoError(err) + + lease := [32]byte{1, 2, 3, 4} + + // construct transactions for sending money to account1 and account2 + // from same sender with identical lease + tx1, err := client.ConstructPayment(account0, account1, 0, 1000000, nil, "", lease, 0, 0) + a.NoError(err) + + stx1, err := client.SignTransactionWithWallet(wh, nil, tx1) + a.NoError(err) + + // submitting the first transaction should succeed + _, err = client.BroadcastTransaction(stx1) + a.NoError(err) + + // wait for the txids and check balance + txids := make(map[string]string) + txids[stx1.Txn.ID().String()] = account0 + + _, curRound := fixture.GetBalanceAndRound(account0) + confirmed := fixture.WaitForAllTxnsToConfirm(curRound+5, txids) + a.True(confirmed, "lease txn confirmed") + + bal1, _ := fixture.GetBalanceAndRound(account1) + bal2, _ := fixture.GetBalanceAndRound(account2) + a.Equal(bal1, uint64(1000000)) + a.Equal(bal2, uint64(0)) + + tx2, err := client.ConstructPayment(account0, account2, 0, 2000000, nil, "", lease, 0, 0) + a.NoError(err) + + stx2, err := client.SignTransactionWithWallet(wh, nil, tx2) + a.NoError(err) + + // submitting the second transaction should fail + _, err = client.BroadcastTransaction(stx2) + a.Error(err) +} + func TestLeaseTransactionsSameSenderDifferentLease(t *testing.T) { t.Parallel() a := require.New(t) var fixture fixtures.RestClientFixture - fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachFuture.json")) + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50Each.json")) defer fixture.Shutdown() client := fixture.LibGoalClient @@ -151,7 +282,7 @@ func TestLeaseTransactionsDifferentSender(t *testing.T) { a := require.New(t) var fixture fixtures.RestClientFixture - fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachFuture.json")) + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50Each.json")) defer fixture.Shutdown() client := fixture.LibGoalClient @@ -225,7 +356,7 @@ func TestOverlappingLeases(t *testing.T) { a := require.New(t) var fixture fixtures.RestClientFixture - fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50EachFuture.json")) + fixture.Setup(t, filepath.Join("nettemplates", "TwoNodes50Each.json")) defer fixture.Shutdown() client := fixture.LibGoalClient diff --git a/test/e2e-go/upgrades/send_receive_upgrade_test.go b/test/e2e-go/upgrades/send_receive_upgrade_test.go index e748a55189..3066db8e4c 100644 --- a/test/e2e-go/upgrades/send_receive_upgrade_test.go +++ b/test/e2e-go/upgrades/send_receive_upgrade_test.go @@ -104,6 +104,20 @@ func TestAccountsCanSendMoneyAcrossUpgradeV15toV16(t *testing.T) { testAccountsCanSendMoneyAcrossUpgrade(t, filepath.Join("nettemplates", "TwoNodes50EachV15Upgrade.json")) } +func TestAccountsCanSendMoneyAcrossUpgradeV21toV22(t *testing.T) { + if runtime.GOOS == "darwin" { + t.Skip() + } + testAccountsCanSendMoneyAcrossUpgrade(t, filepath.Join("nettemplates", "TwoNodes50EachV21Upgrade.json")) +} + +func TestAccountsCanSendMoneyAcrossUpgradeV22toV23(t *testing.T) { + if runtime.GOOS == "darwin" { + t.Skip() + } + testAccountsCanSendMoneyAcrossUpgrade(t, filepath.Join("nettemplates", "TwoNodes50EachV22Upgrade.json")) +} + // ConsensusTestFastUpgrade is meant for testing of protocol upgrades: // during testing, it is equivalent to another protocol with the exception // of the upgrade parameters, which allow for upgrades to take place after @@ -120,6 +134,8 @@ func generateFastUpgradeConsensus() (fastUpgradeProtocols config.ConsensusProtoc fastParams.UpgradeVoteRounds = 5 fastParams.UpgradeThreshold = 3 fastParams.DefaultUpgradeWaitRounds = 5 + fastParams.MinUpgradeWaitRounds = 0 + fastParams.MaxUpgradeWaitRounds = 0 fastParams.MaxVersionStringLen += len(consensusTestFastUpgrade("")) fastParams.ApprovedUpgrades = make(map[protocol.ConsensusVersion]uint64) diff --git a/test/testdata/nettemplates/TwoNodes50EachV22.json b/test/testdata/nettemplates/TwoNodes50EachV22.json new file mode 100644 index 0000000000..4e9dddcbb0 --- /dev/null +++ b/test/testdata/nettemplates/TwoNodes50EachV22.json @@ -0,0 +1,35 @@ +{ + "Genesis": { + "NetworkName": "tbd", + "ConsensusProtocol": "https://github.com/algorandfoundation/specs/tree/57016b942f6d97e6d4c0688b373bb0a2fc85a1a2", + "Wallets": [ + { + "Name": "Wallet1", + "Stake": 50, + "Online": true + }, + { + "Name": "Wallet2", + "Stake": 50, + "Online": true + } + ] + }, + "Nodes": [ + { + "Name": "Primary", + "IsRelay": true, + "Wallets": [ + { "Name": "Wallet1", + "ParticipationOnly": false } + ] + }, + { + "Name": "Node", + "Wallets": [ + { "Name": "Wallet2", + "ParticipationOnly": false } + ] + } + ] +} diff --git a/test/testdata/nettemplates/TwoNodes50EachV22Upgrade.json b/test/testdata/nettemplates/TwoNodes50EachV22Upgrade.json new file mode 100644 index 0000000000..f702f491f5 --- /dev/null +++ b/test/testdata/nettemplates/TwoNodes50EachV22Upgrade.json @@ -0,0 +1,35 @@ +{ + "Genesis": { + "NetworkName": "tbd", + "ConsensusProtocol": "test-fast-upgrade-https://github.com/algorandfoundation/specs/tree/57016b942f6d97e6d4c0688b373bb0a2fc85a1a2", + "Wallets": [ + { + "Name": "Wallet1", + "Stake": 50, + "Online": true + }, + { + "Name": "Wallet2", + "Stake": 50, + "Online": true + } + ] + }, + "Nodes": [ + { + "Name": "Primary", + "IsRelay": true, + "Wallets": [ + { "Name": "Wallet1", + "ParticipationOnly": false } + ] + }, + { + "Name": "Node", + "Wallets": [ + { "Name": "Wallet2", + "ParticipationOnly": false } + ] + } + ] +} From ba4cfef5d5c7b6410659ed37ada6e64255d1f3f0 Mon Sep 17 00:00:00 2001 From: John Lee Date: Thu, 9 Apr 2020 14:53:41 -0400 Subject: [PATCH 2/4] Update Build Number. --- buildnumber.dat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildnumber.dat b/buildnumber.dat index 48082f72f0..b1bd38b62a 100644 --- a/buildnumber.dat +++ b/buildnumber.dat @@ -1 +1 @@ -12 +13 From 097a522c3c9adaecdc0fb5d00d375ccc5bb6d90e Mon Sep 17 00:00:00 2001 From: Tsachi Herman Date: Wed, 8 Apr 2020 12:58:08 -0400 Subject: [PATCH 3/4] Fix TestStoppedCatchupOnUnsupported test bug --- test/e2e-go/features/catchup/basicCatchup_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e-go/features/catchup/basicCatchup_test.go b/test/e2e-go/features/catchup/basicCatchup_test.go index c2ad4df654..5e7c15b8e5 100644 --- a/test/e2e-go/features/catchup/basicCatchup_test.go +++ b/test/e2e-go/features/catchup/basicCatchup_test.go @@ -213,6 +213,7 @@ func TestStoppedCatchupOnUnsupported(t *testing.T) { testUnupgradedProtocol.UpgradeVoteRounds = 3 testUnupgradedProtocol.UpgradeThreshold = 2 testUnupgradedProtocol.DefaultUpgradeWaitRounds = 3 + testUnupgradedProtocol.MinUpgradeWaitRounds = 0 testUnupgradedProtocol.ApprovedUpgrades[consensusTestUnupgradedToProtocol] = 0 consensus[consensusTestUnupgradedProtocol] = testUnupgradedProtocol From 142261c1e63c05ce56d3717e49dd771ef272c39b Mon Sep 17 00:00:00 2001 From: Derek Leung Date: Fri, 3 Apr 2020 18:23:24 -0400 Subject: [PATCH 4/4] Add e2e test for upgrading from v21 to v22. (#949) --- .../TwoNodes50EachV21Upgrade.json | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 test/testdata/nettemplates/TwoNodes50EachV21Upgrade.json diff --git a/test/testdata/nettemplates/TwoNodes50EachV21Upgrade.json b/test/testdata/nettemplates/TwoNodes50EachV21Upgrade.json new file mode 100644 index 0000000000..d5729d5b54 --- /dev/null +++ b/test/testdata/nettemplates/TwoNodes50EachV21Upgrade.json @@ -0,0 +1,35 @@ +{ + "Genesis": { + "NetworkName": "tbd", + "ConsensusProtocol": "test-fast-upgrade-https://github.com/algorandfoundation/specs/tree/8096e2df2da75c3339986317f9abe69d4fa86b4b", + "Wallets": [ + { + "Name": "Wallet1", + "Stake": 50, + "Online": true + }, + { + "Name": "Wallet2", + "Stake": 50, + "Online": true + } + ] + }, + "Nodes": [ + { + "Name": "Primary", + "IsRelay": true, + "Wallets": [ + { "Name": "Wallet1", + "ParticipationOnly": false } + ] + }, + { + "Name": "Node", + "Wallets": [ + { "Name": "Wallet2", + "ParticipationOnly": false } + ] + } + ] +}