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

Added BIP 69: Lexicographical Indexing of Inputs and Outputs #157

Merged
merged 21 commits into from
Sep 19, 2015

Conversation

kristovatlas
Copy link
Contributor

Lexicographical Indexing of Transaction Inputs and Outputs

Lexicographical Indexing of Transaction Inputs and Outputs
@kristovatlas
Copy link
Contributor Author

Electrum 2.3.2 has implemented BIP 69: https://github.com/spesmilo/electrum/blob/master/RELEASE-NOTES

@laanwj
Copy link
Member

laanwj commented Jun 22, 2015

Maybe add reference to discussion on mailing list: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-June/008484.html

@kristovatlas kristovatlas changed the title Added BIP 79 Added BIP 69 Jun 24, 2015
@laanwj
Copy link
Member

laanwj commented Jul 30, 2015

Weird: seems to need rebase

@dcousens
Copy link
Contributor

dcousens commented Aug 7, 2015

LGTM, anything holding this back?

edit: added some comments below


Because the hash of previous transactions and output indices must be included in a signed transaction, wallet clients capable of signing transactions will necessarily have access to this data.

Transaction malleability will not negatively impact the correctness of this process. Even if a wallet client follows this process using unconfirmed UTXOs as inputs and an attacker changes modifies the blockchain’s record of the hash of the previous transaction, the wallet client will include the invalidated previous transaction hash in its input data, and will still correctly sort with respect to that invalidated hash.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is malleability even relevant? I don't foresee this being a common question to ask, but, I guess it doesn't hurt to leave it in.

@dcousens
Copy link
Contributor

dcousens commented Aug 7, 2015

Test fixtures can be found here.

A Javascript implementation can be found here, and on npm.

@dcousens
Copy link
Contributor

dcousens commented Aug 9, 2015

@kristovatlas, @petertodd thoughts on integrating the anti-fee-sniping behaviour (see bitcoin/bitcoin#6216) into BIP69?

@kristovatlas
Copy link
Contributor Author

What's fee sniping?

@dcousens
Copy link
Contributor

@kristovatlas to quote @petertodd from bitcoin/bitcoin#2340

The more important reason is to discourage "fee sniping" by deliberately mining blocks that orphan the current best block. Basically for a large miner the value of the transactions in the best block and the mempool can exceed the cost of deliberately attempting to mine two blocks to orphan the best block. However with nLockTime you'll soon run out of transactions you can put in the first block, which means they now need to go in the second. With limited block sizes you're run out of room, and additionally another miner now only needs to orphan one block to in-turn snipe the high-fee transactions you had to place in the second block, wrecking all your hard work.

TL;DR: stop miners attempting to orphan current blocks in an attempt to increase their fee subsidy reward, enforced by transactions setting their 'minimum' inclusion time as the current block height.

How that is relevant to this BIP is that it does mean transactions are more easily recognized due to nLockTime being set to activeChain.Height() + 1 (the next block).

@kristovatlas
Copy link
Contributor Author

I can sort of see the relationship there, but I'd prefer to keep this BIP straightforward and restricted to advice to Bitcoin wallet clients.

@kristovatlas
Copy link
Contributor Author

Test fixtures can be found here.

A Javascript implementation can be found here, and on npm.

Thanks a lot! I will add links to these in the BIP.

@kristovatlas
Copy link
Contributor Author

I tried rebasing my local copy but to no avail, so I'm going to try submitting a new PR...

@kristovatlas
Copy link
Contributor Author

Looks like I managed to resolve them. Please consider merging, @laanwj

@kristovatlas kristovatlas changed the title Added BIP 69 Added BIP 69: Lexicographical Indexing of Inputs and Outputs Aug 18, 2015
@dcousens
Copy link
Contributor

@kristovatlas any chance of addressing the comments I made above? (on the diff)
If you ignore all the rest, please at least respond to #157 (comment)

@kristovatlas
Copy link
Contributor Author

@dcousens: Thanks for your feedback. I already spent a long time editing the BIP based on prior feedback and I'm not really interested in spending more editing for the sake of succinctness, which comes with trade-offs. When I interacted with open source contributors to wallets on this topic, I found that they primarily wanted more detail about the scheme and not less. Keep in mind that the audience for BIPs is not necessarily restricted to people who religiously read the dev mailing list -- not that you are suggesting this, it's just a helpful reference point.

@dcousens
Copy link
Contributor

@kristovatlas if possible, I'd prefer to stay away from heresay, as, in my experience, I've only heard the opposite in regards to developer opinion on this BIP.

#157 (comment) does not make any sense [to me], although it seems well intentioned, I can't imagine why you have accounted for it, could you clarify that?

The BIP is currently a very long explanation for the following algorithm (pseudocode):

inputs.sort((a, b) => {
  return compare(a.txHash, b.txHash, 32) || a.vout - b.vout
})

outputs.sort((a, b) => {
  return a.value - b.value || compare(a.script, b.script, MIN(a.script.length, b.script.length))
})

Where compare is just a standard lexicographical comparison function, of which there are numerous examples you could give before giving a full python implementation as listed above.

  • std::lexicographical_compare in C++
  • cmp in Python 2.7
  • memcmp in C

Very easy to find, very easy to research.
Almost always pointless to re-implement, or even give a pseudo-algorithm for.

I'd really like to see this BIP get through, but as an open source library implementer, it took me more time to decipher the paragraphs behind this BIP than it did to write the ~20 line npm module.

I'm not trying to be hostile, I'm genuinely just trying to improve the ecosystem, so please don't take these comments the wrong way.

@kristovatlas
Copy link
Contributor Author

Okey doke. I don't think this matters at all and is a waste of my time. If you want to fork and make a pull request to my repo, I will promise to review it promptly, however.

@gmaxwell
Copy link
Contributor

gmaxwell commented Sep 3, 2015

@dcousens Any thoughts on if you will try a revision. What you wrote there looks like a reasonable simplification which could be merged with Kristov's work.

@dcousens
Copy link
Contributor

dcousens commented Sep 3, 2015

@gmaxwell waiting on kristovatlas#1

@kristovatlas
Copy link
Contributor Author

@gmaxwell merged @dcousens's edits. I think this PR is ready to be merged.

@dcousens
Copy link
Contributor

ACK

@kristovatlas
Copy link
Contributor Author

Thanks for the editing help, @dcousens

@kristovatlas
Copy link
Contributor Author

@laanwj ping

1 similar comment
@kristovatlas
Copy link
Contributor Author

@laanwj ping

@luke-jr
Copy link
Member

luke-jr commented Sep 19, 2015

README.mediawiki and bip-0069/bip-0069_examples.py are missing EOF newlines. Anything else needed before merging this draft?

“README.mediawiki and bip-0069/bip-0069_examples.py are missing EOF
newlines. Anything else needed before merging this draft?” — @luke-jr
@kristovatlas
Copy link
Contributor Author

done

luke-jr added a commit that referenced this pull request Sep 19, 2015
Added BIP 69: Lexicographical Indexing of Inputs and Outputs
@luke-jr luke-jr merged commit bde3793 into bitcoin:master Sep 19, 2015
@maflcko
Copy link
Member

maflcko commented Sep 20, 2015

Created two trivial follow up cleanups: #203 and #204. Feedback is welcome.

luke-jr pushed a commit to luke-jr/bips that referenced this pull request Jun 6, 2017
minimum-depth is no longer in the opening message.

Closes: bitcoin#157
Signed-off-by: Rusty Russell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants