💬 Daniel600 commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473612148)
> Philosophical arguments about full RBF aside, this PR proposes deleting an option that is (1) used by a significant amount of nodes and (2) does no harm to the node operator. It would be inappropriate to simply take such an option out from underneath users/downstream software without very strong rationale and a proper deprecation cycle.
Isnt that an argument for not disabling 0-conf
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473612148)
> Philosophical arguments about full RBF aside, this PR proposes deleting an option that is (1) used by a significant amount of nodes and (2) does no harm to the node operator. It would be inappropriate to simply take such an option out from underneath users/downstream software without very strong rationale and a proper deprecation cycle.
Isnt that an argument for not disabling 0-conf
💬 dergoegge commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1473619787)
@vasild Thanks for the review! Took all of your suggestions.
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1473619787)
@vasild Thanks for the review! Took all of your suggestions.
💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1473619930)
ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1473619930)
ACK ec28c89ef69cd3fbb1a82785c841d303b2bf4826
👍 vasild approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261)
(https://github.com/bitcoin/bitcoin/pull/26261)
💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473626748)
Pushed to try to wake the CI up. Looks like it worked!
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473626748)
Pushed to try to wake the CI up. Looks like it worked!
👍 vasild approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK d02b99b10ab6a748fd3c8b0d0faca013ada2d6e3
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK d02b99b10ab6a748fd3c8b0d0faca013ada2d6e3
👍 vasild approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK e29ecece9fed29663cc21caff4b00ceb29ecb959
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK e29ecece9fed29663cc21caff4b00ceb29ecb959
👍 vasild approved a pull request: "rpc, test: remove newline escape sequence from wallet warning fields"
(https://github.com/bitcoin/bitcoin/pull/27138)
ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2
The newline is presentation-specific and does not belong to the server output. For example it will not render as expected if displayed in HTML (where newlines are displayed as a single space). Then it should be `<br/>` instead of `\n`, or maybe `\r\n` for MacOS?
I am happy to review more of the same in another PR or a change that returns an array of warnings so that display engines can chose the best separator for them.
(https://github.com/bitcoin/bitcoin/pull/27138)
ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2
The newline is presentation-specific and does not belong to the server output. For example it will not render as expected if displayed in HTML (where newlines are displayed as a single space). Then it should be `<br/>` instead of `\n`, or maybe `\r\n` for MacOS?
I am happy to review more of the same in another PR or a change that returns an array of warnings so that display engines can chose the best separator for them.
💬 hebasto commented on issue "When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names.":
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-1473654097)
@AdarshRawat1
> can I work on this issue?
This project is permissionless.
Feel free to open a PR :)
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-1473654097)
@AdarshRawat1
> can I work on this issue?
This project is permissionless.
Feel free to open a PR :)
💬 BitcoinErrorLog commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473707197)
> Philosophical arguments about full RBF aside, this PR proposes deleting an option that is (1) used by a significant amount of nodes and (2) does no harm to the node operator. It would be inappropriate to simply take such an option out from underneath users/downstream software without very strong rationale and a proper deprecation cycle.
Where were you when this was happening to first-seen 0conf users? Where was the very strong rationale and proper deprecation cycle then?
Separately, wha
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473707197)
> Philosophical arguments about full RBF aside, this PR proposes deleting an option that is (1) used by a significant amount of nodes and (2) does no harm to the node operator. It would be inappropriate to simply take such an option out from underneath users/downstream software without very strong rationale and a proper deprecation cycle.
Where were you when this was happening to first-seen 0conf users? Where was the very strong rationale and proper deprecation cycle then?
Separately, wha
...
💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140141439)
The following style is more consistent with what we do in other places:
'''cpp
if (!CanGetAddresses(/*internal*/=false)) {
``
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140141439)
The following style is more consistent with what we do in other places:
'''cpp
if (!CanGetAddresses(/*internal*/=false)) {
``
💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473738260)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473738260)
Concept ACK
💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473738719)
Yes, I agree the todo can be removed since there are no other address types to add.
- I think there is no need to add `byte_to_bech32()`, there is an existing encode_segwit_address() function that does the same thing.
- I can use bech32_to_bytes() and encode_segwit_address() with hex encoded string and version as test vectors in test_bech32_decode() just like in test_base58encodedecode().
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473738719)
Yes, I agree the todo can be removed since there are no other address types to add.
- I think there is no need to add `byte_to_bech32()`, there is an existing encode_segwit_address() function that does the same thing.
- I can use bech32_to_bytes() and encode_segwit_address() with hex encoded string and version as test vectors in test_bech32_decode() just like in test_base58encodedecode().
💬 Sjors commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1473745636)
I'm not sure how this is being used in practice, so perhaps the safest thing to do is what @ajtowns suggests: document clearly that `-datacarriersize=1` means "OP_RETURN allowed, but no data" whereas `-nodatacarrier` means "no OP_RETURN allowed at all".
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1473745636)
I'm not sure how this is being used in practice, so perhaps the safest thing to do is what @ajtowns suggests: document clearly that `-datacarriersize=1` means "OP_RETURN allowed, but no data" whereas `-nodatacarrier` means "no OP_RETURN allowed at all".
💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473761343)
@Sjors There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473761343)
@Sjors There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?
💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140178190)
There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140178190)
There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?
💬 Sjors commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473771519)
Very cool. I also noticed [bitcoin.sipa.be/miniscript](https://bitcoin.sipa.be/miniscript/) has been updated.
Has there been any thought into how MuSig2 (and friends) would fit into this in the future? I guess that's only a problem for the compiler, since for the purpose of script it doesn't matter if e.g. public key C is an aggregate of A + B. But it does change the meaning of something like `or(C,and(multi_a(A,B),after(10))`.
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473771519)
Very cool. I also noticed [bitcoin.sipa.be/miniscript](https://bitcoin.sipa.be/miniscript/) has been updated.
Has there been any thought into how MuSig2 (and friends) would fit into this in the future? I guess that's only a problem for the compiler, since for the purpose of script it doesn't matter if e.g. public key C is an aggregate of A + B. But it does change the meaning of something like `or(C,and(multi_a(A,B),after(10))`.
💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473772572)
Thanks for reviewing, I made the update.
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473772572)
Thanks for reviewing, I made the update.
👍 brunoerg approved a pull request: "p2p: Improve diversification of new connections"
(https://github.com/bitcoin/bitcoin/pull/27264)
crACK 72e8ffd7f8dbf908e65da6d012ede914596737ab
(https://github.com/bitcoin/bitcoin/pull/27264)
crACK 72e8ffd7f8dbf908e65da6d012ede914596737ab
💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473775491)
> this PR proposes deleting an option that .. (2) does no harm to the node operator.
This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:
- makes it easier to fingerprint your node and its network peers
- creates pinning vectors
- can allow your counterparties to hide unconfirmed payments to you or hide double-spends of payments to you
- lowers the effectiveness of compact block relay
A
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473775491)
> this PR proposes deleting an option that .. (2) does no harm to the node operator.
This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:
- makes it easier to fingerprint your node and its network peers
- creates pinning vectors
- can allow your counterparties to hide unconfirmed payments to you or hide double-spends of payments to you
- lowers the effectiveness of compact block relay
A
...