💬 instagibbs commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288015854)
personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288015854)
personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion
💬 instagibbs commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288034462)
small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288034462)
small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206253497)
> My concern is not the fact that there are added binaries, but that we force people to use a different one (bitcoin-node, even if it's not visible) if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else.
To be clear, this does not sound like a concern currently, but would be a concern if future PR's do something we **both** agree would be bad: automatically opt users into the process separation featu
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206253497)
> My concern is not the fact that there are added binaries, but that we force people to use a different one (bitcoin-node, even if it's not visible) if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else.
To be clear, this does not sound like a concern currently, but would be a concern if future PR's do something we **both** agree would be bad: automatically opt users into the process separation featu
...
💬 instagibbs commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206261504)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
re-applied all commits and only difference is release notes change location
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206261504)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
re-applied all commits and only difference is release notes change location
💬 stickies-v commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288145855)
+1, small change that might turn out helpful during debugging
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288145855)
+1, small change that might turn out helpful during debugging
👍 stickies-v approved a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136573025)
ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f - all backports clean except the release notes one, as indicated.
Can't see any weird interactions with the 29.x branch.
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136573025)
ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f - all backports clean except the release notes one, as indicated.
Can't see any weird interactions with the 29.x branch.
📝 glozow opened a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226)
Backports #33106 and includes final changes for 29.1rc2. Built on top of #33225. I'll rebase shortly after that's in.
I did not include #32750 because it causes #33177 and I don't foresee any problems; it was just a nice to have.
For reviewers: the backport is unclean but fairly straightforward. I just had to adapt a test that is no longer in master (#32973) and include `-datacarriersize` in order to pad transaction size (#32406).
(https://github.com/bitcoin/bitcoin/pull/33226)
Backports #33106 and includes final changes for 29.1rc2. Built on top of #33225. I'll rebase shortly after that's in.
I did not include #32750 because it causes #33177 and I don't foresee any problems; it was just a nice to have.
For reviewers: the backport is unclean but fairly straightforward. I just had to adapt a test that is no longer in master (#32973) and include `-datacarriersize` in order to pad transaction size (#32406).
🚀 glozow merged a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225)
(https://github.com/bitcoin/bitcoin/pull/33225)
🤔 enirox001 reviewed a pull request: "wallet: Remove isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1
💬 luke-jr commented on pull request "depends: remove xinerama extension from libxcb":
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3206473104)
> (only used for windowing over multiple screens support?)
Deprecated by and incompatible with XRandR which was introduced in ~2001.
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3206473104)
> (only used for windowing over multiple screens support?)
Deprecated by and incompatible with XRandR which was introduced in ~2001.
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288193651)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
As mentioned https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830, would be curious to know if force-system-libcapnp=True works for people, since it seems wasteful to download and build a dependency that we already know must be installed (or the test would be skipped).
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288193651)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
As mentioned https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830, would be curious to know if force-system-libcapnp=True works for people, since it seems wasteful to download and build a dependency that we already know must be installed (or the test would be skipped).
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288166616)
In commit "test: add is_ipc_enabled() and skip_if_no_ipc() functions" (eae32d1a9b5c62707de6afd9832d40de859dd42d)
Not important, but might be better to rename enabled to compiled here to be more similar to surrounding functions which check ENABLE_XXX but are called is_xxx_compiled. (I think your PR actually did this originally but I didn't notice the pattern it was following)
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288166616)
In commit "test: add is_ipc_enabled() and skip_if_no_ipc() functions" (eae32d1a9b5c62707de6afd9832d40de859dd42d)
Not important, but might be better to rename enabled to compiled here to be more similar to surrounding functions which check ENABLE_XXX but are called is_xxx_compiled. (I think your PR actually did this originally but I didn't notice the pattern it was following)
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288233217)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
I might describe this as # Create a remote thread on the server for the IPC calls to be executed in
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288233217)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
I might describe this as # Create a remote thread on the server for the IPC calls to be executed in
👍 ryanofsky approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136603264)
Code review ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f. Looks great! Left some comments that you should feel free to ignore. I am shocked and amazed about how much more friendly the python async API is compared to the c++ API, but I guess coroutines can bring similar benefits to c++. It is also nice how this tests covers different scenarios with waitNext()
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136603264)
Code review ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f. Looks great! Left some comments that you should feel free to ignore. I am shocked and amazed about how much more friendly the python async API is compared to the c++ API, but I guess coroutines can bring similar benefits to c++. It is also nice how this tests covers different scenarios with waitNext()
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288203120)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
Not important, but for easier review and to avoid conflicts in the CI files, it may make sense to split this commit into two commits: one which adds the interface_ipc.py test and allows running it on systems with pycapnp installed, and another commit before or after that installs pycapnp in CI.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288203120)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
Not important, but for easier review and to avoid conflicts in the CI files, it may make sense to split this commit into two commits: one which adds the interface_ipc.py test and allows running it on systems with pycapnp installed, and another commit before or after that installs pycapnp in CI.
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288227488)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
This is probably fine in practice, but it does seem possible for this to fail if the `capnp` binary isn't on the path because `shutil.which` will return None. It might be better to write this as:
````python
if capnp_bin := shutil.which("capnp"):
# Add the system cap'nproto path so include/capnp/c++.capnp can be found.
capnp_dir = Path(shutil.which("capnp")).parent.parent / "include"
...
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288227488)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
This is probably fine in practice, but it does seem possible for this to fail if the `capnp` binary isn't on the path because `shutil.which` will return None. It might be better to write this as:
````python
if capnp_bin := shutil.which("capnp"):
# Add the system cap'nproto path so include/capnp/c++.capnp can be found.
capnp_dir = Path(shutil.which("capnp")).parent.parent / "include"
...
💬 Crypt-iQ commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206481506)
Post-merge ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Only change is release-notes. Tested that rate-limiting works as expected.
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206481506)
Post-merge ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Only change is release-notes. Tested that rate-limiting works as expected.
💬 fanquake commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288289304)
Ok, we can adjust this pre-final.
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288289304)
Ok, we can adjust this pre-final.
💬 glozow commented on pull request "doc: truc packages allow sub min feerate transactions":
(https://github.com/bitcoin/bitcoin/pull/33220#issuecomment-3206570179)
LGTM
(https://github.com/bitcoin/bitcoin/pull/33220#issuecomment-3206570179)
LGTM
💬 maflcko commented on issue "RPC sendmany first (dummy, empty string) argument is not optional":
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206575653)
> ...how do you make a first argument optional, anyway?
You can use named args, by using the `-named=1` option:
```
$ ./bld-cmake/bin/bitcoin-cli -named=1 sendmany amounts='{"hi":1}'
```
This shows that the arg is correctly marked as optional.
What am I missing?
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206575653)
> ...how do you make a first argument optional, anyway?
You can use named args, by using the `-named=1` option:
```
$ ./bld-cmake/bin/bitcoin-cli -named=1 sendmany amounts='{"hi":1}'
```
This shows that the arg is correctly marked as optional.
What am I missing?