Pluralized Transaction Inputs #127

chadlagore · 2017-08-13T05:26:02Z

Status: Ready for review

Related Issue

126

Description

Transactions should reference multiple inputs.
This allows users to coalesce micro-payments into larger ones.

TODOs

Pluralize transactions
Get tests passing again.
Rebase brunos branch.

jordanschalm

Looking good so far 👍

jordanschalm · 2017-08-13T17:17:55Z

blockchain/blockchain.go

@@ -80,3 +90,25 @@ func (bc *BlockChain) CopyBlockByIndex(i uint32) (*Block, error) {
 	}
 	return nil, errors.New("block request out of bounds")
 }
+
+// InputsSpentElsewhere returns true if inputs perported to be only spent


jordanschalm · 2017-08-13T17:28:54Z

blockchain/transaction.go

+// to a transaction.
+func (bc *BlockChain) GetBlockRange(t *Transaction) (uint32, uint32) {
+	min := uint32(math.MaxUint32)
+	max := uint32(math.MaxUint32) // Why I have to cast this? No idea.


should be 0?

Meant to use MinUint32 - nice catch!

jordanschalm · 2017-08-13T17:40:34Z

consensus/consensus.go

@@ -223,9 +222,8 @@ func VerifyBlock(bc *blockchain.BlockChain,
 	// Check for multiple transactions referencing same input transaction.
 	for i, trA := range b.Transactions {


What about:

create input map M for t in transactions: for inp in t.inputs: attempt to add inp to M if already in M: fail

jordanschalm · 2017-08-13T17:46:19Z

pool/pool.go

@@ -53,7 +53,8 @@ func (p *Pool) GetN(N int) *blockchain.Transaction {

 // GetIndex returns the index of the transaction in the ordering.
 func (p *Pool) GetIndex(t *blockchain.Transaction) int {


We lose the property that attempting to add a transaction with an input that already exists will fail. Now we can do [IN1, IN2]->OUT1 and [IN1, IN3]->OUT2 and addition will succeed. What are we doing to mediate this and ensure we don't add to the pool if an input already exists? A many-to-one mapping for inputs -> transactions would be nice. Like IN1->OUT1, IN2->OUT2.

Its actually ok that multiple transactions in the pool exist with the same input. A single transaction may have awarded 2 coins to alice and 1 to bob, that transaction will serve as an input in at most 2 transactions in the future.

I'll admit that this implementation isn't quite perfect though 😬.

The same situation goes for testing double spends within blocks. Its ok that an input is referenced more than once, but not if it is referenced more than once by the same sender. Thanks to your map idea, we now do that check in linear time though 🎉

…ndle multi-inputs

…ref non-existent block

chadlagore · 2017-08-26T04:55:25Z

app/app.go

@@ -351,7 +351,7 @@ func (a *App) RunMiner() {
 		miningResult := miner.Mine(a.Chain, blockToMine)

 		if miningResult.Complete {
-			log.Info("Successfully mined a block!")
+			// log.Debug("Successfully mined a block!")


Quite noisy in tests!

We can just set logging levels in individual tests or in a TestMain.

coveralls · 2017-08-26T04:58:27Z

Coverage decreased (-0.6%) to 76.16% when pulling a605059 on 126-allow-multiple-inputs-in-transactions into 836f6e7 on dev.

coveralls · 2017-08-26T06:10:19Z

Coverage decreased (-0.6%) to 76.16% when pulling d246cb0 on 126-allow-multiple-inputs-in-transactions into 836f6e7 on dev.

coveralls · 2017-08-26T21:34:25Z

Coverage decreased (-0.6%) to 76.16% when pulling fe4a0d6 on 126-allow-multiple-inputs-in-transactions into 836f6e7 on dev.

coveralls · 2017-08-26T21:50:24Z

Coverage decreased (-0.5%) to 76.243% when pulling e24d342 on 126-allow-multiple-inputs-in-transactions into 836f6e7 on dev.

bfbachmann

Extremely high quality 🏅

bfbachmann · 2017-08-26T20:14:47Z

app/app.go

@@ -351,7 +351,7 @@ func (a *App) RunMiner() {
 		miningResult := miner.Mine(a.Chain, blockToMine)

 		if miningResult.Complete {
-			log.Info("Successfully mined a block!")
+			// log.Debug("Successfully mined a block!")


We can just set logging levels in individual tests or in a TestMain.

bfbachmann · 2017-08-26T20:19:27Z

app/app_test.go

@@ -47,7 +47,7 @@ func TestPushHandlerNewTestTransaction(t *testing.T) {
 	select {
 	case tr, ok := <-a.transactionQueue:
 		assert.True(t, ok)
-		assert.Equal(t, tr, txn)
+		assert.ObjectsAreEqual(tr, txn)


Have to use Equal :(

bfbachmann · 2017-08-26T20:22:28Z

app/app_test.go

+	assert.Equal(t, len(a.Chain.Blocks), 2)
+	a.blockQueue <- lastBlock
+	time.Sleep(time.Millisecond * 50)
+	assert.Equal(t, len(a.Chain.Blocks), 2)


Nice tests!

bfbachmann · 2017-08-26T20:23:22Z

blockchain/block.go

@@ -69,7 +69,7 @@ func (b *Block) Len() int {
 }

 // Marshal converts a Block to a byte slice.
-func (b *Block) Marshal() []byte {
+func (b Block) Marshal() []byte {


I actually think it's better to use the pointer receiver here because that way we don't have to copy the block when we call this.

bfbachmann · 2017-08-26T20:31:52Z

blockchain/blockchain.go

+// GetAllInputs returns all the transactions referenced by a transaction
+// as inputs. Returns an error if any of the transactios requested could
+// not be found.
+func (bc *BlockChain) GetAllInputs(t *Transaction) ([]*Transaction, error) {


bfbachmann · 2017-08-26T20:54:23Z

blockchain/test_utils.go

+	tA, _ := TxBody{
+		Sender: sender.Public(),
+		Inputs: []TxHashPointer{},
+		Outputs: []TxOutput{


It might be a good idea to enforce the condition that at least one of the outputs from transaction goes to someone other than the sender.

I'll sleep on this 😴

bfbachmann · 2017-08-26T21:09:28Z

blockchain/transaction.go

+}
+
+// InputsIntersect returns true if the inputs of t intersect with those of other.
+func (t *Transaction) InputsIntersect(other *Transaction) bool {


❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

bfbachmann · 2017-08-26T21:24:09Z

consensus/consensus.go

-		t.TxBody.Input.Hash != blockchain.NilHash ||
-		t.Input.Index != 0 {
+	// Check that the input is 0 (only one input to CB).
+	input := t.TxBody.Inputs[0]


Might want to guard against an out-of-bounds error here.

bfbachmann · 2017-08-26T21:29:42Z

consensus/consensus.go

-	ix := b.BlockNumber - 1
-	if int(ix) > len(bc.Blocks)-1 || ix < 0 {
+	lastBlockIdx := b.BlockNumber - 1
+	if int(lastBlockIdx) != len(bc.Blocks)-1 {


I would change the comment on this function to say something about the fact that we are making sure that this block can be added to the end of the chain.

bfbachmann · 2017-08-26T21:39:40Z

miner/miner.go

@@ -120,8 +120,8 @@ func CloudBase(
 	// Increment the input index of every transaction that has an input in the
 	// new block
 	for _, tx := range b.Transactions[1:] {


I think this for loop should be removed because a transaction can't reference an input in the same block.

coveralls · 2017-08-26T22:52:29Z

Coverage decreased (-0.4%) to 75.965% when pulling 4587504 on 126-allow-multiple-inputs-in-transactions into 6dc8be0 on dev.

pluralized Inputs

049a2ef

chadlagore requested review from david-julien, bfbachmann and jordanschalm August 13, 2017 05:26

chadlagore changed the title ~~Pluralized Transaction Inputs~~ [WIP] Pluralized Transaction Inputs Aug 13, 2017

move intersection function into transaction.go

bca3b8c

jordanschalm reviewed Aug 13, 2017

View reviewed changes

chadlagore added 15 commits August 23, 2017 20:08

Tests passing in blockchain package

44ee8f2

Include set package

64dbf96

Rebuild test fixtures 😅

69d1c15

Added test for GetTotalOutputFor; adjusted InputsSpentElsewhere to ha…

2e882e1

…ndle multi-inputs

Replace MaxUint32 with 0

54fda2f

Fix InputsSpentElsewhere; Respend test passing

765ab7b

All transaction tests passing

efe53f6

All tests pass for blocks

68e956e

Adjust last block check; add asserts

1d8a9c7

Update test for DoubleSpend to allow for many-to-one reference inputs

2c244d5

Replace Len method with Size

b610da9

Reseting glide for merge

735f0f9

Merge dev

57dbb12

Pool updated to handle multi inputs; Handled panic where input index …

08dd363

…ref non-existent block

Fix up app tests to expect new fixture

40592ce

chadlagore changed the title ~~[WIP] Pluralized Transaction Inputs~~ Pluralized Transaction Inputs Aug 26, 2017

Add set package

a605059

chadlagore commented Aug 26, 2017

View reviewed changes

Comments

d246cb0

Replace ObjectsAreEqual and fix debug noise (for real)

fe4a0d6

Remove index bump

e24d342

bfbachmann approved these changes Aug 26, 2017

View reviewed changes

chadlagore added 4 commits August 26, 2017 15:18

Add regression test for indexing issue

2f77508

Reboot glide for merge

5f79262

Merge bruno's work

0ea912a

Update go set package

4587504

chadlagore merged commit 27747c5 into dev Aug 26, 2017

jordanschalm deleted the 126-allow-multiple-inputs-in-transactions branch August 30, 2017 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluralized Transaction Inputs #127

Pluralized Transaction Inputs #127

chadlagore commented Aug 13, 2017 •

edited

Loading

jordanschalm left a comment

jordanschalm Aug 13, 2017

jordanschalm Aug 13, 2017

chadlagore Aug 13, 2017

jordanschalm Aug 13, 2017

jordanschalm Aug 13, 2017

chadlagore Aug 25, 2017

chadlagore Aug 25, 2017 •

edited

Loading

chadlagore Aug 26, 2017

bfbachmann Aug 26, 2017

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

bfbachmann left a comment

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

chadlagore Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

bfbachmann Aug 26, 2017

coveralls commented Aug 26, 2017

		@@ -223,9 +222,8 @@ func VerifyBlock(bc *blockchain.BlockChain,
		// Check for multiple transactions referencing same input transaction.
		for i, trA := range b.Transactions {

		@@ -53,7 +53,8 @@ func (p Pool) GetN(N int) blockchain.Transaction {

		// GetIndex returns the index of the transaction in the ordering.
		func (p Pool) GetIndex(t blockchain.Transaction) int {

Pluralized Transaction Inputs #127

Pluralized Transaction Inputs #127

Conversation

chadlagore commented Aug 13, 2017 • edited Loading

Related Issue

Description

TODOs

jordanschalm 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

chadlagore Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

bfbachmann 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

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

coveralls commented Aug 26, 2017

chadlagore commented Aug 13, 2017 •

edited

Loading

chadlagore Aug 25, 2017 •

edited

Loading