Skip to content

Remove wallet settings from chainparams #16402

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

Jump to bottom
Merged
meshcollider merged 1 commit into from
Jul 27, 2019
Merged

Remove wallet settings from chainparams #16402

meshcollider merged 1 commit into from
Jul 27, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 16, 2019

Feels a bit odd to have wallet setting in the chainparams, so remove them from there

@promag
Copy link
Contributor

promag commented Jul 16, 2019

ACK fa4a605, missed s/2018/2019?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 20, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

darosior
darosior approved these changes Jul 21, 2019
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK fa4a605

@practicalswift
Copy link
Contributor

practicalswift commented Jul 22, 2019

utACK fa4a605

@maflcko
Copy link
Member Author

maflcko commented Jul 25, 2019

@jonasschnelli @jtimon, any objections on this? Otherwise I will merge.

@maflcko maflcko added this to the 0.19.0 milestone Jul 25, 2019
meshcollider
meshcollider reviewed Jul 27, 2019
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

Seems sane, LGTM fa4a605

@meshcollider meshcollider merged commit fa4a605 into bitcoin:master Jul 27, 2019
meshcollider added a commit that referenced this pull request Jul 27, 2019
@meshcollider
Merge #16402: Remove wallet settings from chainparams
dfb7fd6 
fa4a605 Remove wallet settings from chainparams (MarcoFalke)

Pull request description:

  Feels a bit odd to have wallet setting in the chainparams, so remove them from there

ACKs for top commit:
  promag:
    ACK fa4a605, missed s/2018/2019?
  practicalswift:
    utACK fa4a605
  darosior:
    ACK fa4a605

Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
@maflcko maflcko deleted the 1907-walletNoChainParams branch July 27, 2019 14:33
konez2k pushed a commit to bitcoin-green/bitcoingreen that referenced this pull request Jul 27, 2019
@meshcollider @konez2k
Merge bitcoin#16402: Remove wallet settings from chainparams
a0d1c1a 
fa4a605 Remove wallet settings from chainparams (MarcoFalke)

Pull request description:

  Feels a bit odd to have wallet setting in the chainparams, so remove them from there

ACKs for top commit:
  promag:
    ACK fa4a605, missed s/2018/2019?
  practicalswift:
    utACK fa4a605
  darosior:
    ACK fa4a605

Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
@jtimon
Copy link
Contributor

jtimon commented Jul 27, 2019 •
edited
Loading

I dislike using IsTestNet for many things, that was my worry when I complained in #15891 (comment) , but on the other hand I like to remove wallet stuff from chainparams.
I'm not really sure how I feel about this.
Will the bech32 and base58 prefixes do the same?

I guess I would like to hear more opinions about this.

@maflcko
Copy link
Member Author

maflcko commented Jul 27, 2019

Will the bech32 and base58 prefixes do the same?

I think they should stay because they are used even when no wallet is compiled, so they can be considered chain params. Though, chain-specific wallet default values, should, if needed, reside in ./src/wallet/chainparams. Otherwise they will make it harder to move the wallet to a separate repository, since both repos would have to be kept perfectly in sync with regard to the settings.

@jtimon
Copy link
Contributor

jtimon commented Aug 1, 2019

I understand the desire to decouple from the wallet, but the more I think about this, the less I like it.
Now IsTestNet serves for two quite unrelated things. If it was WalletIsTestnet separated from the other one, perhaps it would be better.
Let's say we want regtest to be more like mainnet with respect to the fallback fee, like it happened with the default for acceptnonstdtxs in #15891.
Then we would need to reintroduce m_fallback_fee_enabled or, even worse, introduce an IsRegtest method that ends up being used for all kinds of things.
For decoupling the wallet part of the chainparams, gaving a src/wallet/chainparams sounds like the long term solution. Or perhaps we can simply have the same defaults for mainnet and the test chains for wallet arguments, since testers can set them to whatever they want manually anyway. Perhaps the extra complexity is not worth it in this case.
Either way, I don't see how this helps towards that goal of separating the wallet.

@maflcko
Copy link
Member Author

maflcko commented Aug 1, 2019

m_fallback_fee only exists for testing, if it is deemed that the value should be the same for mainnet, the whole fallback fee logic can be removed instead.

@jtimon jtimon mentioned this pull request Aug 1, 2019
Merged
@jtimon
Copy link
Contributor

jtimon commented Aug 1, 2019 •
edited
Loading

What about doing #16524 ?
That's what I think help the most decoupling wallet and chainparams from each other.
I think the same default as for mainnet should be used, which in practice is currently 0 despite what the constant says. I don't understand why have different defaults for different chains when they're just defaults that can be manually set by the user, or in a config file where I think they truly belong. Much less now that we have sections.
One can just:

[main]
fallbackfee=0

[test]
fallbackfee=20000

[regtest]
fallbackfee=20000

If that's the functionality it ones.

maflcko pushed a commit that referenced this pull request Oct 2, 2019
Merge #16524: Wallet: Disable -fallbackfee by default
a689c11 
ea4cc3a Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)

Pull request description:

  Before it was 0 by default for main and 20000 for test and regtest.
  Now it is 0 by default for all chains, thus there's no need to call Params().

  Also now the default for main is properly documented.

  Suggestion for release notes:

  -fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.

  Should I propose them to the wiki for the release notes or only after merge?

  For more context, see #16402 (comment)

ACKs for top commit:
  MarcoFalke:
    ACK ea4cc3a

Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
@jtimon jtimon mentioned this pull request Oct 3, 2019
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 31, 2020
@majcosta
[backport#16524] Wallet: Disable -fallbackfee by default
cbcbb33 
Summary:
Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)

Pull request description:

  Before it was 0 by default for main and 20000 for test and regtest.
  Now it is 0 by default for all chains, thus there's no need to call Params().

  Also now the default for main is properly documented.

  Suggestion for release notes:

  -fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.

  Should I propose them to the wiki for the release notes or only after merge?

  For more context, see bitcoin/bitcoin#16402 (comment)

bitcoin/bitcoin@ea4cc3a

---

Depends on D7061

Backport of Core [[bitcoin/bitcoin#16524 | PR16524]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7062
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@meshcollider meshcollider meshcollider left review comments

@darosior darosior darosior approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Wallet

Projects

None yet

Milestone

0.19.0

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @promag @DrahtBot @practicalswift @jtimon @meshcollider @darosior