💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1922687448)
I'm working on it. Plan is to harden the function to not read at end and after. I have some initial small draft changes, and want to add testing. Maybe, we can even highlight the issue via additional fuzz+msan tests.
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1922687448)
I'm working on it. Plan is to harden the function to not read at end and after. I have some initial small draft changes, and want to add testing. Maybe, we can even highlight the issue via additional fuzz+msan tests.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475452360)
This could be done in a follow-up if people want it, because currently the conflicting transactions are stored in the `CWalletTx`, and not the transaction state, so this function is not aware of the txids of the mempool conflicts.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475452360)
This could be done in a follow-up if people want it, because currently the conflicting transactions are stored in the `CWalletTx`, and not the transaction state, so this function is not aware of the txids of the mempool conflicts.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453805)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453805)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453968)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475453968)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454363)
Thanks, that should be fixed now
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454363)
Thanks, that should be fixed now
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454524)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454524)
Done
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454837)
Good point, the tests now check the exact contents.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475454837)
Good point, the tests now check the exact contents.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475455088)
I've added an assertion that the tx isn't in the mempool.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475455088)
I've added an assertion that the tx isn't in the mempool.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475457535)
Yes, I will add these in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1475457535)
Yes, I will add these in a follow-up PR.
💬 DanaBCannon commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1475536100)
Im Open For Business
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1475536100)
Im Open For Business
💬 stratospher commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922955178)
tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour!
there is also`p2p_leak.py`, `p2p_timeouts.py`, `p2p_tx_privacy.py` where `on_version()` is used with `add_p2p_connection()`/inbound to `TestNode` connections which aren't affected by this. I guess we should think about it only if we end up doing outbound to `TestNode` connections there.
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1922955178)
tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour!
there is also`p2p_leak.py`, `p2p_timeouts.py`, `p2p_tx_privacy.py` where `on_version()` is used with `add_p2p_connection()`/inbound to `TestNode` connections which aren't affected by this. I guess we should think about it only if we end up doing outbound to `TestNode` connections there.
💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1923061666)
> @petertodd
>
> > Can you give a bit more detail on what challenges you think that'll pose?
>
> from my memory: "How this new replacement rule would behave if you have a parent in the "replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic. i wond
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1923061666)
> @petertodd
>
> > Can you give a bit more detail on what challenges you think that'll pose?
>
> from my memory: "How this new replacement rule would behave if you have a parent in the "replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic. i wond
...
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923151784)
> I've heard similar arguments from many people, and I have to say I disagree. The miners need to follow the will of the users (especially the holders), or bitcoin cannot survive long-term. Mining pools are highly attuned to short-term interests, generally at the expense of long-term considerations. This is understandable given the competitive and unpredictable nature of proof-of-work pooled mining. A pool operator who tries to be a good citizen and caretake bitcoin's long-term interests will mo
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923151784)
> I've heard similar arguments from many people, and I have to say I disagree. The miners need to follow the will of the users (especially the holders), or bitcoin cannot survive long-term. Mining pools are highly attuned to short-term interests, generally at the expense of long-term considerations. This is understandable given the competitive and unpredictable nature of proof-of-work pooled mining. A pool operator who tries to be a good citizen and caretake bitcoin's long-term interests will mo
...
💬 BitcoinMechanic commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923175031)
The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
As you point out, it is **extremely easy** to get three or four entities to do something like mempoolfullrbf=1.
The difference here is we aren't trying to route around network defaults, we are trying to have them be set
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923175031)
The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
As you point out, it is **extremely easy** to get three or four entities to do something like mempoolfullrbf=1.
The difference here is we aren't trying to route around network defaults, we are trying to have them be set
...
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923231248)
> The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
If it's so trivial, you should do that first: get a majority of hash power to set `-permitbaremultisig=0`. If you can, then you'd have a much better argument for this pull-req.
> As you point out, it is **extremely
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923231248)
> The issue with (I hope) accidental conflation of "miners" with "pool operators" as you are doing @petertodd is that it makes a trivial task (getting a dozen people to flip a `1` to a `0`) seem insurmountable by implying that we are dealing with miners in general. We aren't.
If it's so trivial, you should do that first: get a majority of hash power to set `-permitbaremultisig=0`. If you can, then you'd have a much better argument for this pull-req.
> As you point out, it is **extremely
...
💬 BitcoinMechanic commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923269102)
>If it's so trivial, you should do that first: get a majority of hash power to set -permitbaremultisig=0. If you can, then you'd have a much better argument for this pull-req.
Lobbying mining pools in order to backwards-rationalize and shoehorn changes in to bitcoin core itself is perhaps more efficient in that you get to take advantage of the awful centralization issue we have with pools currently, but is clearly not the correct way to go about things.
If you're going to assert that the c
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1923269102)
>If it's so trivial, you should do that first: get a majority of hash power to set -permitbaremultisig=0. If you can, then you'd have a much better argument for this pull-req.
Lobbying mining pools in order to backwards-rationalize and shoehorn changes in to bitcoin core itself is perhaps more efficient in that you get to take advantage of the awful centralization issue we have with pools currently, but is clearly not the correct way to go about things.
If you're going to assert that the c
...
💬 stratospher commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1923275862)
> Another possible alternative for addconnection's third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);).
@theStack, i went with the mandatory third parameter approach because:
1. this can be used only for interaction between python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) on only regtest anyways.
2. we're indirectly passing whatever i
...
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1923275862)
> Another possible alternative for addconnection's third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);).
@theStack, i went with the mandatory third parameter approach because:
1. this can be used only for interaction between python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) on only regtest anyways.
2. we're indirectly passing whatever i
...
💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1475695984)
> I'll look into replace `write` with `>>`.
`>>` means reading. You may want to try `<<`, if you want to write an object.
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1475695984)
> I'll look into replace `write` with `>>`.
`>>` means reading. You may want to try `<<`, if you want to write an object.
💬 maflcko commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1923278780)
I think it is fine to not have an abort command for now, because currently it is also not possible to abort, unless the complete process is shut down. But up to you.
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1923278780)
I think it is fine to not have an abort command for now, because currently it is also not possible to abort, unless the complete process is shut down. But up to you.
💬 maflcko commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1923279799)
lgtm
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1923279799)
lgtm