💬 Sjors commented on pull request "[25.x] Parallel compact block downloads":
(https://github.com/bitcoin/bitcoin/pull/27752#issuecomment-1565420134)
utACK 50c86f57af5c6a9aa2e4828aeb67d641340b0860
(https://github.com/bitcoin/bitcoin/pull/27752#issuecomment-1565420134)
utACK 50c86f57af5c6a9aa2e4828aeb67d641340b0860
💬 MarcoFalke commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1565431844)
Not sure. My preference would be to figure out why the blocks are of different size and then make that deterministic instead.
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1565431844)
Not sure. My preference would be to figure out why the blocks are of different size and then make that deterministic instead.
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208014798)
I can't remember tbh, try to find it in the history with no success, not sure if I did it due to matching the consumers(?) or @vasild suggested it at some point...
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208014798)
I can't remember tbh, try to find it in the history with no success, not sure if I did it due to matching the consumers(?) or @vasild suggested it at some point...
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208014857)
Well, I think that an empty string wouldn't be a valid URI, at least the `scheme component` (the first part of it which is followed by a ":", which is again followed by the `hier-part` component (which may be empty) must be present according to the [RFC](https://www.rfc-editor.org/rfc/rfc3986#section-3). I thought it was perhaps validated inside the `evhttp_uri_parse` but I couldn't find it [there](https://github.com/libevent/libevent/blob/bca26524fc4cd7a9e79d210c1079baaa7d29835d/http.c#L4464-L4
...
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208014857)
Well, I think that an empty string wouldn't be a valid URI, at least the `scheme component` (the first part of it which is followed by a ":", which is again followed by the `hier-part` component (which may be empty) must be present according to the [RFC](https://www.rfc-editor.org/rfc/rfc3986#section-3). I thought it was perhaps validated inside the `evhttp_uri_parse` but I couldn't find it [there](https://github.com/libevent/libevent/blob/bca26524fc4cd7a9e79d210c1079baaa7d29835d/http.c#L4464-L4
...
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015048)
Yeah missed them, done now, thanks!
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015048)
Yeah missed them, done now, thanks!
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015177)
Yeah and also it'll be more consistent with the comments above in the private declaration section.
https://github.com/bitcoin/bitcoin/blob/c72967f48ab9eb02b8dc9231fdcc6ee85d66b7b9/src/httpserver.h#L60-L63
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015177)
Yeah and also it'll be more consistent with the comments above in the private declaration section.
https://github.com/bitcoin/bitcoin/blob/c72967f48ab9eb02b8dc9231fdcc6ee85d66b7b9/src/httpserver.h#L60-L63
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015277)
Ok, done.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1208015277)
Ok, done.
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1565460712)
Updates:
- Incorporated most of @stickies-v suggestions
- I'll remove the `httpserver` private member `replySent` (as @vasild suggested and @stickies-v agrees with), the argument from the constructor and all the logic around it in a separated commit. I can squash it later into 1 if you like.
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1565460712)
Updates:
- Incorporated most of @stickies-v suggestions
- I'll remove the `httpserver` private member `replySent` (as @vasild suggested and @stickies-v agrees with), the argument from the constructor and all the logic around it in a separated commit. I can squash it later into 1 if you like.
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207644980)
when defining classes in python, don't we skip parentheses?
using `class ECPubKey:` and `class ECKey:` instead of `class ECPubKey():` and `class ECKey():`
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207644980)
when defining classes in python, don't we skip parentheses?
using `class ECPubKey:` and `class ECKey:` instead of `class ECPubKey():` and `class ECKey():`
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207999486)
never mind, just reviewed key.py and saw how this is being used in `verify_schnorr`, `tweak_add_pubkey`. agree that it's better/simpler to keep it as it is.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207999486)
never mind, just reviewed key.py and saw how this is being used in `verify_schnorr`, `tweak_add_pubkey`. agree that it's better/simpler to keep it as it is.
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207758013)
I thought of "The result is cached." as:
1. someone calls `int()`
2. we do operations inside `int()`, compute what num and den should be and store it locally+lazily without updating actual `self.num`, `self.den`
3. someone calls another FE function
4. the newly computed value of `self.num`, `self.den` gets actually updated only here
isn't this what caching a variable conceptually means? (even though it doesn't matter here where `self.num`, `self.den` gets actually updated) what did y
...
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1207758013)
I thought of "The result is cached." as:
1. someone calls `int()`
2. we do operations inside `int()`, compute what num and den should be and store it locally+lazily without updating actual `self.num`, `self.den`
3. someone calls another FE function
4. the newly computed value of `self.num`, `self.den` gets actually updated only here
isn't this what caching a variable conceptually means? (even though it doesn't matter here where `self.num`, `self.den` gets actually updated) what did y
...
💬 brunoerg commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1208033507)
Instead of pushing the node to `vNodes` and removing it if it's not routable, I think you could do something like:
```cpp
static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType, bool onion_peer = false)
{
CAddress addr;
if (onion_peer) {
auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)};
BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr)));
}
wh
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1208033507)
Instead of pushing the node to `vNodes` and removing it if it's not routable, I think you could do something like:
```cpp
static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType, bool onion_peer = false)
{
CAddress addr;
if (onion_peer) {
auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)};
BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr)));
}
wh
...
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1565503469)
Ok nice, great. Thanks for the details.
We should code a test exercising exactly that behavior.
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1565503469)
Ok nice, great. Thanks for the details.
We should code a test exercising exactly that behavior.
👍 furszy approved a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1447436505)
Tested ACK 89df7987
> For example, let's say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob's node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob's node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob's mapTxSpe
...
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1447436505)
Tested ACK 89df7987
> For example, let's say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob's node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob's node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob's mapTxSpe
...
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1565558335)
> Would be good to have a real functional test in the future.
#27307 tests this behavior, which is how I found this bug.
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1565558335)
> Would be good to have a real functional test in the future.
#27307 tests this behavior, which is how I found this bug.
🚀 achow101 merged a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145)
(https://github.com/bitcoin/bitcoin/pull/27145)
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1208070951)
great feedback! that's much cleaner. incorporated in the latest push
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1208070951)
great feedback! that's much cleaner. incorporated in the latest push
💬 RandyMcMillan commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1565610693)
ACK 4492de1be11f4131811f504a037342c78f480e20
post merge ACK
Great job!
https://github.com/bitcoin-core/gui/pull/701#issuecomment-1401350037
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1565610693)
ACK 4492de1be11f4131811f504a037342c78f480e20
post merge ACK
Great job!
https://github.com/bitcoin-core/gui/pull/701#issuecomment-1401350037
💬 recursive-rat4 commented on issue "Allow accepting non-standard transactions on mainnet via local rpc":
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1565665282)
>The scenario that I have in mind though is alternative transport mechanisms for bitcoin transactions that exist outside the p2p network and that are publicly accessible. Miners are incentivised to pull out of that source, and therefore regular nodes should do that too to keep their mempool as much in sync as possible?
Miner's incentives are more complex than simply put into blockchain anything that pays a satoshi. One may end up in a situation where mempool is filled up with a type of non-st
...
(https://github.com/bitcoin/bitcoin/issues/27768#issuecomment-1565665282)
>The scenario that I have in mind though is alternative transport mechanisms for bitcoin transactions that exist outside the p2p network and that are publicly accessible. Miners are incentivised to pull out of that source, and therefore regular nodes should do that too to keep their mempool as much in sync as possible?
Miner's incentives are more complex than simply put into blockchain anything that pays a satoshi. One may end up in a situation where mempool is filled up with a type of non-st
...
💬 furszy commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1565677870)
> Not sure. My preference would be to figure out why the blocks are of different size and then make that deterministic instead.
I started there but.. do we really care? (aside from dev curiosity, where I agree that wouldn't be bad to know why).
The test goal is to exercise `getblockfrompeer` and not pruning nor where blocks are stored. Having all those hardcoded heights, expecting to have a precise number of blocks in each block file, looks quite fragile.
I mean, all the hardcoded `prun
...
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1565677870)
> Not sure. My preference would be to figure out why the blocks are of different size and then make that deterministic instead.
I started there but.. do we really care? (aside from dev curiosity, where I agree that wouldn't be bad to know why).
The test goal is to exercise `getblockfrompeer` and not pruning nor where blocks are stored. Having all those hardcoded heights, expecting to have a precise number of blocks in each block file, looks quite fragile.
I mean, all the hardcoded `prun
...