💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253624315)
I’ve been doing some staring at this today. It seems to me that the waste score of finished input sets should also be adjusted according to the bump fee discount. Gotta do more pondering, will get back to this.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253624315)
I’ve been doing some staring at this today. It seems to me that the waste score of finished input sets should also be adjusted according to the bump fee discount. Gotta do more pondering, will get back to this.
💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253454337)
Yes, thanks, I removed the earlier instances of ComputeAndSetWaste where the results get returned from the various coin selection algorithms and instead of selectively updating here, always calculate it here now. I just forgot to remove the comment.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253454337)
Yes, thanks, I removed the earlier instances of ComputeAndSetWaste where the results get returned from the various coin selection algorithms and instead of selectively updating here, always calculate it here now. I just forgot to remove the comment.
🤔 achow101 reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1515315141)
I think we aren't handling missing `id`s correctly?
From the spec:
> id
> An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]
When there is no `id`, we should treat the as a notification and can just ignore the request, rather than responding to it as we currently do. It seems like i
...
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1515315141)
I think we aren't handling missing `id`s correctly?
From the spec:
> id
> An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]
When there is no `id`, we should treat the as a notification and can just ignore the request, rather than responding to it as we currently do. It seems like i
...
💬 achow101 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1253614618)
In d11fb70b3fea7c1be5f48e32837d2c81dc7cbd10 "test: cover more HTTP error codes in interface_rpc.py"
This seems unnecessary?
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1253614618)
In d11fb70b3fea7c1be5f48e32837d2c81dc7cbd10 "test: cover more HTTP error codes in interface_rpc.py"
This seems unnecessary?
💬 jonatack commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1622495909)
@pinheadmz I'd do the simplest thing, or what @achow101 thinks on this and regarding progress on the https://github.com/bitcoin/bitcoin/issues/20160 timeline.
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1622495909)
@pinheadmz I'd do the simplest thing, or what @achow101 thinks on this and regarding progress on the https://github.com/bitcoin/bitcoin/issues/20160 timeline.
🤔 furszy reviewed a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1515410110)
> > (An alternative, functionally equivalent to this, might be to just make the timeout infinite, see [#27988 (comment)](https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007), but I think this is fine as well.)
>
> I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and uni
...
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1515410110)
> > (An alternative, functionally equivalent to this, might be to just make the timeout infinite, see [#27988 (comment)](https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007), but I think this is fine as well.)
>
> I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and uni
...
💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253680561)
> test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused
Agree, I see that as well with Python 3.11.4.
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253680561)
> test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused
Agree, I see that as well with Python 3.11.4.
👋 mzumsande's pull request is ready for review: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114)
(https://github.com/bitcoin/bitcoin/pull/26114)
💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687)
Looking at this again, the following would move the string interpolation outside the loop (with perhaps clearer naming). Feel free to ignore.
```diff
- us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
- info = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
+ sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
+ our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
+
...
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687)
Looking at this again, the following would move the string interpolation outside the loop (with perhaps clearer naming). Feel free to ignore.
```diff
- us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
- info = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
+ sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
+ our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
+
...
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622575068)
Did some more manual testing and it worked as intended for me - moved out of draft!
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622575068)
Did some more manual testing and it worked as intended for me - moved out of draft!
💬 jonatack commented on pull request "Supporting parameter "h" and "?" in -netinfo.":
(https://github.com/bitcoin/bitcoin/pull/27830#discussion_r1253696102)
I don't have a strong opinion for or against on this proposed change, but the documentation for this user interface is provided by `bitcoin-cli -help`
```
$ ./src/bitcoin-cli -help | grep -A4 netinfo
-netinfo
Get network peer connection information from the remote server. An
optional integer argument from 0 to 4 can be passed for different
peers listings (default: 0). Pass "help" for detailed help
documentation.
```
and so this change would involve upd
...
(https://github.com/bitcoin/bitcoin/pull/27830#discussion_r1253696102)
I don't have a strong opinion for or against on this proposed change, but the documentation for this user interface is provided by `bitcoin-cli -help`
```
$ ./src/bitcoin-cli -help | grep -A4 netinfo
-netinfo
Get network peer connection information from the remote server. An
optional integer argument from 0 to 4 can be passed for different
peers listings (default: 0). Pass "help" for detailed help
documentation.
```
and so this change would involve upd
...
💬 jonatack commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622588498)
Concept ACK, will have a look at this (and merged https://github.com/bitcoin/bitcoin/pull/27577 soon).
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622588498)
Concept ACK, will have a look at this (and merged https://github.com/bitcoin/bitcoin/pull/27577 soon).
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253703097)
Oh, yuck; that's a bad rebase. :(
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253703097)
Oh, yuck; that's a bad rebase. :(
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253707986)
That's moving a member of a local struct/object, not a simple local, so it can't have been constructed in place and copy/move elision isn't possible, though?
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253707986)
That's moving a member of a local struct/object, not a simple local, so it can't have been constructed in place and copy/move elision isn't possible, though?
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253709698)
Ah, you may be right.
https://shaharmike.com/cpp/rvo/#returning-member
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253709698)
Ah, you may be right.
https://shaharmike.com/cpp/rvo/#returning-member
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253739563)
<details><summary>If this test is valid (arm64 clang 16), returning the struct member without std::move may be better</summary><p>
```cpp
// rvo.cpp
#include <iostream>
struct Snitch { // Note: All methods have side effects
Snitch() { std::cout << "ctor" << std::endl; }
~Snitch() { std::cout << "dtor" << std::endl; }
Snitch(const Snitch&) { std::cout << "copy ctor" << std::endl; }
Snitch(Snitch&&) { std::cout << "move ctor" << std::endl; }
Snitch& operato
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253739563)
<details><summary>If this test is valid (arm64 clang 16), returning the struct member without std::move may be better</summary><p>
```cpp
// rvo.cpp
#include <iostream>
struct Snitch { // Note: All methods have side effects
Snitch() { std::cout << "ctor" << std::endl; }
~Snitch() { std::cout << "dtor" << std::endl; }
Snitch(const Snitch&) { std::cout << "copy ctor" << std::endl; }
Snitch(Snitch&&) { std::cout << "move ctor" << std::endl; }
Snitch& operato
...
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253743213)
For wtxid relay peers the prior `.insert(hash)` line adds the wtxid to the filter; for non-wtxid relay peers, it adds the txid to the filter. We only need to do a second insert to add the txid for wtxid relay peers, and then only when the wtxid is actually different to the txid. The extra check is just because testing a bool is easier than comparing two uint256s.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253743213)
For wtxid relay peers the prior `.insert(hash)` line adds the wtxid to the filter; for non-wtxid relay peers, it adds the txid to the filter. We only need to do a second insert to add the txid for wtxid relay peers, and then only when the wtxid is actually different to the txid. The extra check is just because testing a bool is easier than comparing two uint256s.
📝 achow101 reopened a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)
(Non-GUI part of #15987, but without the bloom stuff)
(https://github.com/bitcoin/bitcoin/pull/22693)
(Non-GUI part of #15987, but without the bloom stuff)
💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1253745180)
That would break the wallettool again
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1253745180)
That would break the wallettool again
💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1622665055)
Rebased.
>if I understand correctly, this PR only makes it easier to tell if you're reusing one of your own wallet addresses for receiving, it doesn't tell you if you're reusing an address you've already used to send to someone else.
> do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?
As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1622665055)
Rebased.
>if I understand correctly, this PR only makes it easier to tell if you're reusing one of your own wallet addresses for receiving, it doesn't tell you if you're reusing an address you've already used to send to someone else.
> do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?
As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).