📝 fanquake locked a pull request: "Pending changes exported from your codespace"
(https://github.com/bitcoin/bitcoin/pull/31128)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31128)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
📝 fanquake locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/31126)
This pull request updates the `README.md` file to improve its readability by formatting the first line as a markdown header. The change involves adding a hashtag at the beginning of the line, transforming "Bitcoin Core integration/staging tree" into "# Bitcoin Core integration/staging tree". This modification enhances the document structure, making it easier for users to identify the title at a glance.
No changes have been made to functions, global data structures, or external interfaces. Th
...
(https://github.com/bitcoin/bitcoin/pull/31126)
This pull request updates the `README.md` file to improve its readability by formatting the first line as a markdown header. The change involves adding a hashtag at the beginning of the line, transforming "Bitcoin Core integration/staging tree" into "# Bitcoin Core integration/staging tree". This modification enhances the document structure, making it easier for users to identify the title at a glance.
No changes have been made to functions, global data structures, or external interfaces. Th
...
:lock: fanquake locked an issue: "Bitcoin"
(https://github.com/bitcoin/bitcoin/issues/31129)
(https://github.com/bitcoin/bitcoin/issues/31129)
🚀 fanquake merged a pull request: "rpc: net: follow-ups for #30062"
(https://github.com/bitcoin/bitcoin/pull/30183)
(https://github.com/bitcoin/bitcoin/pull/30183)
👍 TheCharlatan approved a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2384578222)
lgtm ACK 92ed1bd586f7048f75b38024521696ff04e5f69c
Though @stickies-v's [suggestion](92ed1bd586f7048f75b38024521696ff04e5f69c) would be a nice addition. This was non-obvious to me.
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2384578222)
lgtm ACK 92ed1bd586f7048f75b38024521696ff04e5f69c
Though @stickies-v's [suggestion](92ed1bd586f7048f75b38024521696ff04e5f69c) would be a nice addition. This was non-obvious to me.
🤔 rkrux reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2384597940)
Post Merge tACK 98c1536852d1de9a978b11046e7414e79ed40b46
Successful make and functional tests.
I tried to test this RPC in my local by connecting 2 nodes in the regtest env. However, I couldn't get an orphan transaction generated. I tried 2 ways:
1. Set `mempoolexpiry` to 1hr in one node and broadcast the child transaction 15mins after the parent transaction from the other node. Initially I expected the parent transaction to get dropped after an hour but now it didn't. My guess it's becau
...
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2384597940)
Post Merge tACK 98c1536852d1de9a978b11046e7414e79ed40b46
Successful make and functional tests.
I tried to test this RPC in my local by connecting 2 nodes in the regtest env. However, I couldn't get an orphan transaction generated. I tried 2 ways:
1. Set `mempoolexpiry` to 1hr in one node and broadcast the child transaction 15mins after the parent transaction from the other node. Initially I expected the parent transaction to get dropped after an hour but now it didn't. My guess it's becau
...
💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428794373)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/26334.
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428794373)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/26334.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2428837974)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2428837974)
Rebased.
💬 brunoerg commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2428844570)
> One thing to consider is that this PR changes the REST interface from being read-only to also being able to "modify" node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.
Yes. Could it bring a security issue? Maybe someone could use this endpoint maliciously to spend resources?
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2428844570)
> One thing to consider is that this PR changes the REST interface from being read-only to also being able to "modify" node state (mempool in this case). This PR might introduce new considerations for someone exposing the REST interface to a local or even a public network: Someone who previously could only query information can now also publish transactions via the node.
Yes. Could it bring a security issue? Maybe someone could use this endpoint maliciously to spend resources?
💬 ismaelsadeeq commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1810430953)
```suggestion
* Note: In certain rare edge cases, monotonically increasing estimates may not be
* guaranteed. Specifically, given two targets N and M, where M > N, if a sub-estimate
* for target N fail to return a valid fee rate, while target M has valid fee rate for that
* sub-estimate, target M may result in a higher fee rate
* estimate than target N.
*
* See: https://github.com/bitcoin/bitcoin/issues/11800#issuecomment-349697807
```
I think the s
...
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1810430953)
```suggestion
* Note: In certain rare edge cases, monotonically increasing estimates may not be
* guaranteed. Specifically, given two targets N and M, where M > N, if a sub-estimate
* for target N fail to return a valid fee rate, while target M has valid fee rate for that
* sub-estimate, target M may result in a higher fee rate
* estimate than target N.
*
* See: https://github.com/bitcoin/bitcoin/issues/11800#issuecomment-349697807
```
I think the s
...
🤔 ismaelsadeeq reviewed a pull request: "fees: document non-monotonic estimation edge case"
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2384720527)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31080#pullrequestreview-2384720527)
Concept ACK
💬 fanquake commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#issuecomment-2428976762)
Two other things that should happen at the same time as this are adding the linker (this is somewhat compensated for via adding the c/xx flags), as well as making the flag overriding work correctly.
(https://github.com/bitcoin/bitcoin/pull/31125#issuecomment-2428976762)
Two other things that should happen at the same time as this are adding the linker (this is somewhat compensated for via adding the c/xx flags), as well as making the flag overriding work correctly.
💬 fanquake commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428995399)
> It is definitely broken in https://github.com/bitcoin/bitcoin/pull/30997.
Shouldn't #30997 be based on this PR then (or at least mention in the PR description it's expected to be broken)?
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2428995399)
> It is definitely broken in https://github.com/bitcoin/bitcoin/pull/30997.
Shouldn't #30997 be based on this PR then (or at least mention in the PR description it's expected to be broken)?
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1810527928)
In e0b79e029f15cf2e984c513e230b0d7c7914554d (build: Remove problematic code ): Can you add a sentence explaining why it's problematic?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1810527928)
In e0b79e029f15cf2e984c513e230b0d7c7914554d (build: Remove problematic code ): Can you add a sentence explaining why it's problematic?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2429007032)
`6b10008441...ec7693a9aa`: take suggestions from https://github.com/bitcoin/bitcoin/pull/29420 - in the SOCKS5 proxy, put the sockets forwarding snippet from `socks5.py` to its own function and retry `send()`s.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2429007032)
`6b10008441...ec7693a9aa`: take suggestions from https://github.com/bitcoin/bitcoin/pull/29420 - in the SOCKS5 proxy, put the sockets forwarding snippet from `socks5.py` to its own function and retry `send()`s.
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#issuecomment-2429009514)
`f93fab4c58...57529ac4db`: address suggestions
(https://github.com/bitcoin/bitcoin/pull/29420#issuecomment-2429009514)
`f93fab4c58...57529ac4db`: address suggestions
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810539480)
I moved it to its own function, but in `test/functional/test_framework/socks5.py` because it uses the newly created `sendall()` (due to the [suggestion below](https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1802683914)) which is very similar to `recvall()` which already exists in `socks5.py`.
I guess if this is to be moved to `test/functional/test_framework/netutil.py`, then all 3 of `recvall()`, `sendall()` and `forward_sockets()` should be moved. Leave it as is or move to `netutil
...
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810539480)
I moved it to its own function, but in `test/functional/test_framework/socks5.py` because it uses the newly created `sendall()` (due to the [suggestion below](https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1802683914)) which is very similar to `recvall()` which already exists in `socks5.py`.
I guess if this is to be moved to `test/functional/test_framework/netutil.py`, then all 3 of `recvall()`, `sendall()` and `forward_sockets()` should be moved. Leave it as is or move to `netutil
...
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810559925)
Ok, [`sendall()`](https://docs.python.org/3/library/socket.html#socket.socket.sendall) doc says "On error, an exception is raised, and there is no way to determine how much data, if any, was successfully sent" so this is useless for us. The doc says as well that since Python 3.5 the method will retry if the syscall is interrupted, but mentions nothing about `EAGAIN` or `EWOULDBLOCK`.
So I followed https://docs.python.org/3/howto/sockets.html#using-a-socket and used the basic `send()` with a m
...
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810559925)
Ok, [`sendall()`](https://docs.python.org/3/library/socket.html#socket.socket.sendall) doc says "On error, an exception is raised, and there is no way to determine how much data, if any, was successfully sent" so this is useless for us. The doc says as well that since Python 3.5 the method will retry if the syscall is interrupted, but mentions nothing about `EAGAIN` or `EWOULDBLOCK`.
So I followed https://docs.python.org/3/howto/sockets.html#using-a-socket and used the basic `send()` with a m
...
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810578522)
Following discussion with @tdb3, how do we know this code even works and is not totally broken? That is a reasonable question because this PR does not demonstrate that this works.
https://github.com/bitcoin/bitcoin/pull/29415 (which includes this PR) exercises the newly added functionality. The tests from https://github.com/bitcoin/bitcoin/pull/29415 that use this depend on the private broadcast functionality so can't be included here.
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1810578522)
Following discussion with @tdb3, how do we know this code even works and is not totally broken? That is a reasonable question because this PR does not demonstrate that this works.
https://github.com/bitcoin/bitcoin/pull/29415 (which includes this PR) exercises the newly added functionality. The tests from https://github.com/bitcoin/bitcoin/pull/29415 that use this depend on the private broadcast functionality so can't be included here.
💬 Christewart commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2429112141)
ACK bf46723537c37cb1ee7f84ffe7b75c253beadb89
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2429112141)
ACK bf46723537c37cb1ee7f84ffe7b75c253beadb89