💬 glozow commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457194322)
nit: replace magic number with `MAX_STANDARD_TX_WEIGHT`
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457194322)
nit: replace magic number with `MAX_STANDARD_TX_WEIGHT`
💬 glozow commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457200003)
Would this be giving up too early? We could still find a different solution further down the function, no?
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457200003)
Would this be giving up too early? We could still find a different solution further down the function, no?
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1898164704)
Hm the CI failure of `previous releases, qt5 dev package and depends packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing.
Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1898164704)
Hm the CI failure of `previous releases, qt5 dev package and depends packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing.
Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1898213458)
Thank you for looking this over @ryanofsky,
Updated 487b12e18ce007379a997da48b5ee97f459f6342 -> e3592ee55756c08bda3075b54cbff719d3fb964f ([noGlobalSignals_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_3) -> [noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_3..noGlobalSignals_4))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#issuec
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1898213458)
Thank you for looking this over @ryanofsky,
Updated 487b12e18ce007379a997da48b5ee97f459f6342 -> e3592ee55756c08bda3075b54cbff719d3fb964f ([noGlobalSignals_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_3) -> [noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_3..noGlobalSignals_4))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#issuec
...
💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457258511)
I think wrapping the trace parameter in `static_cast<int32_t>()` is the correct way to resolve this because `change_pos` is otherwise a `std::optional<unsigned int>`. In tracing.md `change_pos` is defined as int32 so it should always be returned as a signed 32-bit int.
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457258511)
I think wrapping the trace parameter in `static_cast<int32_t>()` is the correct way to resolve this because `change_pos` is otherwise a `std::optional<unsigned int>`. In tracing.md `change_pos` is defined as int32 so it should always be returned as a signed 32-bit int.
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898282534)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging? afaik the biggest unnecessary bloater of the UTXO set is the stacks protocol.
Normal users only see what gets highlighted in famous explorers and discussed on twitter. I think the best way to "filter" everything is `blocksonly=1` as suggested by a few other developers.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898282534)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging? afaik the biggest unnecessary bloater of the UTXO set is the stacks protocol.
Normal users only see what gets highlighted in famous explorers and discussed on twitter. I think the best way to "filter" everything is `blocksonly=1` as suggested by a few other developers.
💬 stickies-v commented on pull request "[26.x] more backports":
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898291449)
Backports LGTM d055adad985752c5ae3cf9dd0f509c83850e6c5b but it seems that the commit messages are different than usual? It doesn't seem like we have formal documentation on this, but keeping things uniform helps keep scripts simple (e.g. compiling a list of all pulls that are backported from `git log`) so this might be worth updating?
- `Rebased-From` only mentions the SHA when I think typically we use the full hash (needed for `git fetch`)
- `Github-Pull` uses `bitcoin#<number>` when I think
...
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898291449)
Backports LGTM d055adad985752c5ae3cf9dd0f509c83850e6c5b but it seems that the commit messages are different than usual? It doesn't seem like we have formal documentation on this, but keeping things uniform helps keep scripts simple (e.g. compiling a list of all pulls that are backported from `git log`) so this might be worth updating?
- `Rebased-From` only mentions the SHA when I think typically we use the full hash (needed for `git fetch`)
- `Github-Pull` uses `bitcoin#<number>` when I think
...
💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457305630)
.. or maybe `static_cast` is overkill, a cast with `int32_t()` seems to be what's recommended in developer-nodes.md .
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457305630)
.. or maybe `static_cast` is overkill, a cast with `int32_t()` seems to be what's recommended in developer-nodes.md .
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898302003)
I got to ask, as even a maintainer has brought this up. What is stopping the demand for P2MS from moving to P2PK? IMO this solution is ineffective due to a lack of comprehensiveness.
*insert meme of gate with no fence*
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898302003)
I got to ask, as even a maintainer has brought this up. What is stopping the demand for P2MS from moving to P2PK? IMO this solution is ineffective due to a lack of comprehensiveness.
*insert meme of gate with no fence*
💬 glozow commented on pull request "log: Don't use scientific notation in "Dumped mempool" message":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1898313413)
> plain `%.3f` will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.
Why? "It took less than 1ms" should be pretty intuitive.
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1898313413)
> plain `%.3f` will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.
Why? "It took less than 1ms" should be pretty intuitive.
💬 vasild commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1898337593)
`6a51eaf4a9...d0c8109dd0`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-1898337593)
`6a51eaf4a9...d0c8109dd0`: rebase due to conflicts
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898338611)
> What is stopping the demand for P2MS from moving to P2PK?
Or P2WSH
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898338611)
> What is stopping the demand for P2MS from moving to P2PK?
Or P2WSH
💬 Earnestly commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898346948)
> with some tweaking, could probably even fit in an `OP_RETURN`.
Which could then be discarded by node operators, demonstrating its purpose.
The 80 byte limit on `OP_RETURN` is a filter, it is one which can be bypassed simply by having a miner include transactions which exceed it out of band. Why is this not deemed "censorship" and why does bitcoin core bother placing an 80 byte limit on it?
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898346948)
> with some tweaking, could probably even fit in an `OP_RETURN`.
Which could then be discarded by node operators, demonstrating its purpose.
The 80 byte limit on `OP_RETURN` is a filter, it is one which can be bypassed simply by having a miner include transactions which exceed it out of band. Why is this not deemed "censorship" and why does bitcoin core bother placing an 80 byte limit on it?
🤔 ismaelsadeeq reviewed a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175#pullrequestreview-1829533448)
Code review ACK 51fdb4819ebda4d4a8f64c8d36a471af65a033c9
Just a single nit, happy to re ACK when fixed.
I've tested this on regtest and it passes as expected.
(https://github.com/bitcoin/bitcoin/pull/29175#pullrequestreview-1829533448)
Code review ACK 51fdb4819ebda4d4a8f64c8d36a471af65a033c9
Just a single nit, happy to re ACK when fixed.
I've tested this on regtest and it passes as expected.
💬 ismaelsadeeq commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1457349966)
nit:
iwyu
```cpp
#include <util/strencodings.h>
```
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1457349966)
nit:
iwyu
```cpp
#include <util/strencodings.h>
```
💬 ismaelsadeeq commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1898369428)
> I believe CI failures are unrelated. Is it possible to force CI to re-run?
It has to do with #29234, If you rebase on master I think the CI will be happy, but IMO there is no need since it's unrelated
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1898369428)
> I believe CI failures are unrelated. Is it possible to force CI to re-run?
It has to do with #29234, If you rebase on master I think the CI will be happy, but IMO there is no need since it's unrelated
👍 ismaelsadeeq approved a pull request: "fuzz: make FuzzedDataProvider usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1829550326)
ACK 01960c53c7d71c70792abe19413315768dc2275a
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1829550326)
ACK 01960c53c7d71c70792abe19413315768dc2275a
💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1457369802)
Circling back to this, I still don't think the interface choices in this PR are right.
Mostly it's just that the `NetEventsInterface` is not the place for a getter like this. It is meant for networking events (hence the name) such as opening/closing connections and processing/sending messages. So Approach NACK on misusing this interface.
> The node's contextual information and logic required to take the decision of opening a socket to a particular address resides mostly in PeerManager (rig
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1457369802)
Circling back to this, I still don't think the interface choices in this PR are right.
Mostly it's just that the `NetEventsInterface` is not the place for a getter like this. It is meant for networking events (hence the name) such as opening/closing connections and processing/sending messages. So Approach NACK on misusing this interface.
> The node's contextual information and logic required to take the decision of opening a socket to a particular address resides mostly in PeerManager (rig
...
💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898392767)
> Which could then be discarded by node operators, demonstrating its purpose.
It wont be discarded by default because they use less than 80 bytes for JSON. Could use even lesser bytes by changing the protocol.
> The 80 byte limit on OP_RETURN is a filter, it is one which can be bypassed simply by having a miner include transactions which exceed it out of band. Why is this not deemed "censorship" and why does bitcoin core bother placing an 80 byte limit on it?
Related pull request and a
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898392767)
> Which could then be discarded by node operators, demonstrating its purpose.
It wont be discarded by default because they use less than 80 bytes for JSON. Could use even lesser bytes by changing the protocol.
> The 80 byte limit on OP_RETURN is a filter, it is one which can be bypassed simply by having a miner include transactions which exceed it out of band. Why is this not deemed "censorship" and why does bitcoin core bother placing an 80 byte limit on it?
Related pull request and a
...
💬 Retropex commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898413591)
> It wont be discarded by default because they use less than 80 bytes for JSON
You persist in not understanding that the goal is not only to protect the network from spam but also to give the node runners the tools to choose their mempool policies.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898413591)
> It wont be discarded by default because they use less than 80 bytes for JSON
You persist in not understanding that the goal is not only to protect the network from spam but also to give the node runners the tools to choose their mempool policies.