Skip to content

Conversation

sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Apr 7, 2016

Before activation, such transactions might not be reliably mined, so don't allow into the mempool.

Please tag for backport to 0.12.1

@morcos
Copy link
Contributor

morcos commented Apr 7, 2016

utACK

@petertodd
Copy link
Contributor

utACK da5fdbb

@jonasschnelli
Copy link
Contributor

utACK da5fdbb

@paveljanik
Copy link
Contributor

ACK da5fdbb

@btcdrak
Copy link
Contributor

btcdrak commented Apr 7, 2016

tACK da5fdbb

// Don't relay version 2 transactions until CSV is active, and we can be
// sure that such transactions will be mined (unless we're on
// -testnet/-regtest).
const CChainParams& chainparams = Params();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to the top of the function (other parts of the function before this also call Params() and could use the variable in the future)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing above in this function calls Params(); I guess I didn't check the functions called from here to see if they call Params(), but can we please fix that later when we change those functions? This has many ACKs now and is holding up 0.12.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sdaftuar I agree, it's not important for this pull.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was in another branch, You are right for master. If you need to change the code for any other reason, I would appreciate moving this line too though.

@dcousens
Copy link
Contributor

dcousens commented Apr 8, 2016

utACK da5fdbb, but agreed with @jtimon.

@jl2012
Copy link
Contributor

jl2012 commented Apr 8, 2016

tACK da5fdbb

@CodeShark
Copy link
Contributor

utACK da5fdbb

@NicolasDorier
Copy link
Contributor

utACK da5fdbb

@laanwj laanwj merged commit da5fdbb into bitcoin:master Apr 8, 2016
laanwj added a commit that referenced this pull request Apr 8, 2016
…ivates

da5fdbb Test relay of version 2 transactions (Suhas Daftuar)
5cb1d8a Tests: move get_bip9_status to util.py (Suhas Daftuar)
e4ba9f6 Version 2 transactions remain non-standard until CSV activates (Suhas Daftuar)
laanwj pushed a commit that referenced this pull request Apr 8, 2016
Before activation, such transactions might not be mined, so don't
allow into the mempool.

- Tests: move get_bip9_status to util.py

- Test relay of version 2 transactions

Github-Pull: #7835
Rebased-From: e4ba9f6 5cb1d8a da5fdbb
laanwj added a commit that referenced this pull request Apr 8, 2016
@maflcko
Copy link
Member

maflcko commented Jun 9, 2016

This was backported as 46898e7. Removing label

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.