💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838074248)
Ah RIP, this is a merge conflict with #30592. That's the CI error.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838074248)
Ah RIP, this is a merge conflict with #30592. That's the CI error.
💬 ryanofsky commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2470510859)
Code review d9cfb2a314caa1cdcae8e47a03dad557ad96e069.
This generally looks good except commit 2d65e6ae0756b87e82dbafd032308e8687ec7941 disallowing `-noconf`. Specifying `-noconf` to disable reading from the default config file path is useful and something that should be supported, even if now it's only working in a very quirky way by treating the datadir as an empty configuration file. I think a better implementation would not look too different though, maybe like:
```diff
--- a/src/commo
...
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2470510859)
Code review d9cfb2a314caa1cdcae8e47a03dad557ad96e069.
This generally looks good except commit 2d65e6ae0756b87e82dbafd032308e8687ec7941 disallowing `-noconf`. Specifying `-noconf` to disable reading from the default config file path is useful and something that should be supported, even if now it's only working in a very quirky way by treating the datadir as an empty configuration file. I think a better implementation would not look too different though, maybe like:
```diff
--- a/src/commo
...
💬 AndreaDiazCorreia commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2470534984)
I've reviewed and tested the listmempooltransactions RPC implementation. Here's what I did:
- Pulled the branch and rebased it on top of master
- Tested it manually on regtest both with and without transactions in the mempool
- Created a functional test for this RPC. You can find it in my repo at https://github.com/AndreaDiazCorreia/bitcoin/tree/PR29016-listmempooltransactions
The test verifies:
- Basic mempool sequence behavior
- Transaction tracking
- Sequence filtering functionalit
...
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2470534984)
I've reviewed and tested the listmempooltransactions RPC implementation. Here's what I did:
- Pulled the branch and rebased it on top of master
- Tested it manually on regtest both with and without transactions in the mempool
- Created a functional test for this RPC. You can find it in my repo at https://github.com/AndreaDiazCorreia/bitcoin/tree/PR29016-listmempooltransactions
The test verifies:
- Basic mempool sequence behavior
- Transaction tracking
- Sequence filtering functionalit
...
💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2470557837)
Right! Looks like for `Discover()` to run `-discover` must be enabled and `-bind` not used. It is tough to explain what a config like `-discover=1 -bind=1.2.3.4:5555` would do. Seems that this is another thing that can be simplified.
Also, this has another flaw: `bind_on_any` will be `false` if `-bind=0.0.0.0:5555` is used, but it should really be `true` :-/
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2470557837)
Right! Looks like for `Discover()` to run `-discover` must be enabled and `-bind` not used. It is tough to explain what a config like `-discover=1 -bind=1.2.3.4:5555` would do. Seems that this is another thing that can be simplified.
Also, this has another flaw: `bind_on_any` will be `false` if `-bind=0.0.0.0:5555` is used, but it should really be `true` :-/
💬 vasild commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2470593900)
@darosior, good observations and questions! Maybe use the P2P-seeds only as a fallback if it takes too long?
I mean this - right now there are around 2000 hardcoded addresses in `contrib/seeds/nodes_main.txt`. Some of them will be down, thus the problem mentioned above: "Another downside would be that finding an initial batch of peers would take longer". So, maybe if a new node can't find enough peers in some reasonable time from those 2000 (and GETADDR replies if it manages to connect to som
...
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2470593900)
@darosior, good observations and questions! Maybe use the P2P-seeds only as a fallback if it takes too long?
I mean this - right now there are around 2000 hardcoded addresses in `contrib/seeds/nodes_main.txt`. Some of them will be down, thus the problem mentioned above: "Another downside would be that finding an initial batch of peers would take longer". So, maybe if a new node can't find enough peers in some reasonable time from those 2000 (and GETADDR replies if it manages to connect to som
...
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838156911)
I meant `-bind=` as a means to disable the onion bind on `127.0.0.1:8334` to avoid the port collision, but I typed `-bind=...=onion` instead :(
One can use both depending on what's needed: `-bind=1.2.3.4:5555 -bind=127.0.0.1:8335=onion`.
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838156911)
I meant `-bind=` as a means to disable the onion bind on `127.0.0.1:8334` to avoid the port collision, but I typed `-bind=...=onion` instead :(
One can use both depending on what's needed: `-bind=1.2.3.4:5555 -bind=127.0.0.1:8335=onion`.
📝 theStack opened a pull request: "doc: mention `descriptorprocesspsbt` in psbt.md"
(https://github.com/bitcoin/bitcoin/pull/31277)
Noticed that the `descriptorprocesspsbt` RPC call is currently not documented anywhere.
(https://github.com/bitcoin/bitcoin/pull/31277)
Noticed that the `descriptorprocesspsbt` RPC call is currently not documented anywhere.
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838170305)
> Do you think that is an issue we should care about?
No.
I have found simple/dumb programs easier to configure compared to programs that try to be intelligent on behalf of the user.
> Shaw's Principle:
> Build a system that even a fool can use, and only a fool will want to use it.
This is a bit subjective and I may have a bias here, but if I put `Port 80` in `/etc/ssh/sshd_config` I can't imagine sshd saying something like "Hey, you are binding sshd on port 80, your web ser
...
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838170305)
> Do you think that is an issue we should care about?
No.
I have found simple/dumb programs easier to configure compared to programs that try to be intelligent on behalf of the user.
> Shaw's Principle:
> Build a system that even a fool can use, and only a fool will want to use it.
This is a bit subjective and I may have a bias here, but if I put `Port 80` in `/etc/ssh/sshd_config` I can't imagine sshd saying something like "Hey, you are binding sshd on port 80, your web ser
...
💬 m3dwards commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2470652675)
> Any idea what was wrong with the syntax I used?
The `|| github.event_name == 'pull_request'` is code that needs to be executed rather than a static data so it needs to be inside the double quotes. I believe the hardcoded string of `|| github.event_name == 'pull_request'` is truthy hence it would run the job. Github Actions is finicky!
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2470652675)
> Any idea what was wrong with the syntax I used?
The `|| github.event_name == 'pull_request'` is code that needs to be executed rather than a static data so it needs to be inside the double quotes. I believe the hardcoded string of `|| github.event_name == 'pull_request'` is truthy hence it would run the job. Github Actions is finicky!
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838181981)
I don't know how useful `-port=5555 -bind=127.0.0.1=onion` is or how many people use it. But it is supported on `master` and thus we better not brick it. And this makes our lives harder.
Out of the scope of this PR, I am in favor of dropping support for `-bind=addresswithoutport`. Just to make things simpler for us and maybe also for the users that will not have to wonder what happens if some weird combination of options are used, e.g. `port=1111 bind=2.2.2.2:3333`.
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1838181981)
I don't know how useful `-port=5555 -bind=127.0.0.1=onion` is or how many people use it. But it is supported on `master` and thus we better not brick it. And this makes our lives harder.
Out of the scope of this PR, I am in favor of dropping support for `-bind=addresswithoutport`. Just to make things simpler for us and maybe also for the users that will not have to wonder what happens if some weird combination of options are used, e.g. `port=1111 bind=2.2.2.2:3333`.
📝 polespinasa opened a pull request: "Wallet, RPC: Settxfeerate"
(https://github.com/bitcoin/bitcoin/pull/31278)
This PR aims to solve https://github.com/bitcoin/bitcoin/issues/31088
New RPC call ``settxfeerate`` is added. It does the same as the actual ``settxfee`` but takes the feerate in sat/vB instead of B/kvB.
``settxfee`` still available but hidden so we avoid incompatibility problems with applications that use that RPC call.
When deprecation is desired, the process should be as simple as just delete ``settxfee`` RPC call function and correct the tests where it is called.
Some simple tests
...
(https://github.com/bitcoin/bitcoin/pull/31278)
This PR aims to solve https://github.com/bitcoin/bitcoin/issues/31088
New RPC call ``settxfeerate`` is added. It does the same as the actual ``settxfee`` but takes the feerate in sat/vB instead of B/kvB.
``settxfee`` still available but hidden so we avoid incompatibility problems with applications that use that RPC call.
When deprecation is desired, the process should be as simple as just delete ``settxfee`` RPC call function and correct the tests where it is called.
Some simple tests
...
💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2470664552)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2470664552)
concept ACK
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1838191729)
> Do you mind sharing the code of your attempt (assuming it compiles :))? I wanna take a look, but would be useful for archival reasons anyway.
It compiled, but it won't pass tests because I would have had to heavily modify the BroadcastTransaction interface. Not sure I kept the branch sorry.
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1838191729)
> Do you mind sharing the code of your attempt (assuming it compiles :))? I wanna take a look, but would be useful for archival reasons anyway.
It compiled, but it won't pass tests because I would have had to heavily modify the BroadcastTransaction interface. Not sure I kept the branch sorry.
💬 polespinasa commented on pull request "Wallet, RPC: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2470674762)
Please note that the 2 commits are different approaches of the same solution.
I personally prefer 50a2783a20390121dcf66bdbce350c6435773b18 as the two functions are independent and deprecating will be easier.
b96cfae733a8a8eb79ee894a7cbe903fa955bcec is based on ``HandleRequest`` to forward the call to the original ``settxfee`` function. This gave me problems with how to handle error codes and also functions aren't independent, so I think it's a worse approach.
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2470674762)
Please note that the 2 commits are different approaches of the same solution.
I personally prefer 50a2783a20390121dcf66bdbce350c6435773b18 as the two functions are independent and deprecating will be easier.
b96cfae733a8a8eb79ee894a7cbe903fa955bcec is based on ``HandleRequest`` to forward the call to the original ``settxfee`` function. This gave me problems with how to handle error codes and also functions aren't independent, so I think it's a worse approach.
💬 vasild commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2470680219)
> It's important to make this clear in the release notes.
Note that the problem this PR is trying to resolve has a release note and an easy workaround. We are here because some people (many?) didn't read the release notes even after encountering the problem (second bitcoind refuses to start because of port collision). A startup failure is an error that cannot be ignored. Whereas a silent breakage of the hidden service could remain unnoticed for a long time - one would have to check the number
...
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2470680219)
> It's important to make this clear in the release notes.
Note that the problem this PR is trying to resolve has a release note and an easy workaround. We are here because some people (many?) didn't read the release notes even after encountering the problem (second bitcoind refuses to start because of port collision). A startup failure is an error that cannot be ignored. Whereas a silent breakage of the hidden service could remain unnoticed for a long time - one would have to check the number
...
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470681103)
rebased due to conflict with merge of https://github.com/bitcoin/bitcoin/pull/30592
going to go over all outstanding comments
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470681103)
rebased due to conflict with merge of https://github.com/bitcoin/bitcoin/pull/30592
going to go over all outstanding comments
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2470706357)
The functional test runs correctly on my local environment:
``64/313 - rpc_settxfeerate.py passed, Duration: 1 s``
I don't know if anything needs to be done in the IC to run the tests correctly.
Help regarding this is greatly appreciated.
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2470706357)
The functional test runs correctly on my local environment:
``64/313 - rpc_settxfeerate.py passed, Duration: 1 s``
I don't know if anything needs to be done in the IC to run the tests correctly.
Help regarding this is greatly appreciated.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838215999)
should be the same, or even better since it's not being fetched from the mempool?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838215999)
should be the same, or even better since it's not being fetched from the mempool?
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838230470)
If it's not in mempool or the package, then it can't have unconfirmed dust. Is there a scenario you're concerned about?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838230470)
If it's not in mempool or the package, then it can't have unconfirmed dust. Is there a scenario you're concerned about?
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838232378)
might be heretical but I find as-is easier to read? Can add to follow-up if demand is there.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838232378)
might be heretical but I find as-is easier to read? Can add to follow-up if demand is there.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838237284)
I probably used it more than once before, will remove in follow-up
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838237284)
I probably used it more than once before, will remove in follow-up