💬 achow101 commented on pull request "fuzz: BIP 42, BIP 30, CVE-2018-17144":
(https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1522062568)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1522062568)
Are you still working on this?
📝 achow101 converted_to_draft a pull request: "Make all networking code mockable"
(https://github.com/bitcoin/bitcoin/pull/21878)
_This is a roadmap PR. It can be merged, but it can also be split into separate PRs and to get proper thorough review it is split._
Add wrapper methods to the syscalls `accept()`, `setsockopt()`, `getsockname()`, `bind()`, `listen()` in the [`Sock`](https://github.com/bitcoin/bitcoin/blob/eb9a1fe03779bf05062b70f14190cb23ff42b46f/src/util/sock.h#L25) class (e.g. `Sock::Accept()`). Those methods can be overriden (mocked) by unit tests ([existent example in `master`](https://github.com/bitcoin/
...
(https://github.com/bitcoin/bitcoin/pull/21878)
_This is a roadmap PR. It can be merged, but it can also be split into separate PRs and to get proper thorough review it is split._
Add wrapper methods to the syscalls `accept()`, `setsockopt()`, `getsockname()`, `bind()`, `listen()` in the [`Sock`](https://github.com/bitcoin/bitcoin/blob/eb9a1fe03779bf05062b70f14190cb23ff42b46f/src/util/sock.h#L25) class (e.g. `Sock::Accept()`). Those methods can be overriden (mocked) by unit tests ([existent example in `master`](https://github.com/bitcoin/
...
💬 real-or-random commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1522066480)
> Based on those results, if at all, those optimizations should probably only be enabled if GCC is used? (Not sure if it's worth the review and maintenance burden though.)
Some of the code changes here conflict with https://github.com/bitcoin/bitcoin/pull/21590, and given that clang 14 is better than this ASM, we should maybe get benchmarks on a recent GCC (like 12.2) and see if it's faster. If not, I also wonder if there's a better way to convince GCC to output good code.
@theStack Can y
...
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1522066480)
> Based on those results, if at all, those optimizations should probably only be enabled if GCC is used? (Not sure if it's worth the review and maintenance burden though.)
Some of the code changes here conflict with https://github.com/bitcoin/bitcoin/pull/21590, and given that clang 14 is better than this ASM, we should maybe get benchmarks on a recent GCC (like 12.2) and see if it's faster. If not, I also wonder if there's a better way to convince GCC to output good code.
@theStack Can y
...
💬 achow101 commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1522069254)
This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1522069254)
This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.
✅ achow101 closed a pull request: "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN"
(https://github.com/bitcoin/bitcoin/pull/27086)
(https://github.com/bitcoin/bitcoin/pull/27086)
📝 achow101 converted_to_draft a pull request: "util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/17783)
**This is based on #16545 + #17580.** The non-base commits are:
- [`cf2c60c5203` util: Forbid ambiguous multiple assignments in config file](https://github.com/bitcoin/bitcoin/pull/17493/commits/cf2c60c5203c67e23f396d82b2edb09bb7e01f59)
- [`20ace2d0964` test: Extend util_ArgsMerge test to check for "Multiple values specified" errors](https://github.com/bitcoin/bitcoin/pull/17493/commits/20ace2d096420974689fe18b8fa73d120d7c6d10)
---
Make negating list options act the same as not specify
...
(https://github.com/bitcoin/bitcoin/pull/17783)
**This is based on #16545 + #17580.** The non-base commits are:
- [`cf2c60c5203` util: Forbid ambiguous multiple assignments in config file](https://github.com/bitcoin/bitcoin/pull/17493/commits/cf2c60c5203c67e23f396d82b2edb09bb7e01f59)
- [`20ace2d0964` test: Extend util_ArgsMerge test to check for "Multiple values specified" errors](https://github.com/bitcoin/bitcoin/pull/17493/commits/20ace2d096420974689fe18b8fa73d120d7c6d10)
---
Make negating list options act the same as not specify
...
📝 achow101 converted_to_draft a pull request: "refactor: Remove settings merge reverse precedence code"
(https://github.com/bitcoin/bitcoin/pull/17581)
**This is based on #16545 + #17580 + #17493.** The non-base commits are:
- [`a057e0c38f2` refactor: Remove settings merge reverse precedence code](https://github.com/bitcoin/bitcoin/pull/17581/commits/a057e0c38f2d56219568bc6f4ff9e972cdb339c0)
---
This has no effect on behavior because as of https://github.com/bitcoin/bitcoin/pull/17493 it's not possible to specify multiple values for single value settings in the config file.
This change implements one of the settings simplifications
...
(https://github.com/bitcoin/bitcoin/pull/17581)
**This is based on #16545 + #17580 + #17493.** The non-base commits are:
- [`a057e0c38f2` refactor: Remove settings merge reverse precedence code](https://github.com/bitcoin/bitcoin/pull/17581/commits/a057e0c38f2d56219568bc6f4ff9e972cdb339c0)
---
This has no effect on behavior because as of https://github.com/bitcoin/bitcoin/pull/17493 it's not possible to specify multiple values for single value settings in the config file.
This change implements one of the settings simplifications
...
📝 achow101 converted_to_draft a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859)
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
(https://github.com/bitcoin/bitcoin/pull/26859)
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
💬 achow101 commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1522073245)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1522073245)
Are you still working on this?
💬 ekzyis commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1522078187)
> Would arguably be better to do such checks by signing a transaction and not broadcasting it.
I think having to create a transaction is unnecessary complex if you just want to check if you can spend a UTXO.
For a transaction, you need to specify amount, fee rate and receiving address. And after signing, you must make sure to wipe this transaction completely from your computer so no one else can broadcast it.
> You don't fight bullies with hiding and give up things. You push back like peo
...
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1522078187)
> Would arguably be better to do such checks by signing a transaction and not broadcasting it.
I think having to create a transaction is unnecessary complex if you just want to check if you can spend a UTXO.
For a transaction, you need to specify amount, fee rate and receiving address. And after signing, you must make sure to wipe this transaction completely from your computer so no one else can broadcast it.
> You don't fight bullies with hiding and give up things. You push back like peo
...
✅ achow101 closed a pull request: "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
(https://github.com/bitcoin/bitcoin/pull/25621)
(https://github.com/bitcoin/bitcoin/pull/25621)
💬 achow101 commented on pull request "wallet: Guard against undefined behaviour":
(https://github.com/bitcoin/bitcoin/pull/25982#issuecomment-1522082927)
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
(https://github.com/bitcoin/bitcoin/pull/25982#issuecomment-1522082927)
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
✅ achow101 closed a pull request: "wallet: Guard against undefined behaviour"
(https://github.com/bitcoin/bitcoin/pull/25982)
(https://github.com/bitcoin/bitcoin/pull/25982)
💬 achow101 commented on pull request "refactor: make some BlockManager members const":
(https://github.com/bitcoin/bitcoin/pull/26664#issuecomment-1522083768)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.
Closing due to lack of interest. Pull requests with improvements are always welcome.
(https://github.com/bitcoin/bitcoin/pull/26664#issuecomment-1522083768)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.
Closing due to lack of interest. Pull requests with improvements are always welcome.
✅ achow101 closed a pull request: "refactor: make some BlockManager members const"
(https://github.com/bitcoin/bitcoin/pull/26664)
(https://github.com/bitcoin/bitcoin/pull/26664)
💬 achow101 commented on pull request "refactor: replace all implicit C-style const/const+reinterpret with explicit casts":
(https://github.com/bitcoin/bitcoin/pull/27126#issuecomment-1522085388)
This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.
(https://github.com/bitcoin/bitcoin/pull/27126#issuecomment-1522085388)
This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.
✅ achow101 closed a pull request: "refactor: replace all implicit C-style const/const+reinterpret with explicit casts"
(https://github.com/bitcoin/bitcoin/pull/27126)
(https://github.com/bitcoin/bitcoin/pull/27126)
💬 ekzyis commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1176819779)
nit:
I would use "or" after "a network/netmask (e.g. 1.2.3.4/255.255.255.0)" now:
```diff
- argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0), a network/CIDR (e.g. 1.2.3.4/24, 0.0.0.0/0 (all ipv4) or ::/0 (all ipv6)). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
+ argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RP
...
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1176819779)
nit:
I would use "or" after "a network/netmask (e.g. 1.2.3.4/255.255.255.0)" now:
```diff
- argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0), a network/CIDR (e.g. 1.2.3.4/24, 0.0.0.0/0 (all ipv4) or ::/0 (all ipv6)). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
+ argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RP
...
💬 satsie commented on issue "Add per message stats to getnettotals rpc":
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1522205393)
Thanks @vasild! I think `show_only` is really clear and straightforward. I will update my code to use that instead.
In the process of updating, I noticed the phrase "aggregated by" used in the context of the getpeerinfo RPC, [where it breaks results down by message type](https://github.com/bitcoin/bitcoin/blob/v25.0rc1/src/rpc/net.cpp#L152):
```
{RPCResult::Type::OBJ_DYN, "bytessent_per_msg", "",
{
{RPCResult::Type::NUM, "m
...
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1522205393)
Thanks @vasild! I think `show_only` is really clear and straightforward. I will update my code to use that instead.
In the process of updating, I noticed the phrase "aggregated by" used in the context of the getpeerinfo RPC, [where it breaks results down by message type](https://github.com/bitcoin/bitcoin/blob/v25.0rc1/src/rpc/net.cpp#L152):
```
{RPCResult::Type::OBJ_DYN, "bytessent_per_msg", "",
{
{RPCResult::Type::NUM, "m
...
💬 TheCharlatan commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1522212627)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1522212627)
Concept ACK