💬 amitiuttarwar commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1214617536)
yeah, not the best but I think the chances of repeatedly not hitting the `Assume` condition to be extremely low. alternatively, I could add a counter to prevent that worst case if it seems worth the additional complexity.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214627508)
@amitiuttarwar sort of. It's two things:

1. If enum values are not explicitly assigned (which is the case for the `Network` enum right now), there is no guarantee that `NET_MAX` is what you think it will be. This came out of a conversation with @vasild during review earlier this year. It's my understanding that `NET_MAX` _probably_ corresponds to the total number of enum values, but even today there are no guarantees on the value of `NET_MAX`. I'd like to do a little more research here and ge
...

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214629720)
Good point. Do you think it's okay to do

```
if (index >= NUM_NET_MESSAGE_TYPES) {
return NET_MESSAGE_TYPE_OTHER;
}
```

Or should I add a separate case for `if (index > NUM_NET_MESSAGE_TYPES)` that throws an error? Not in love with this because it's harder to get test coverage, but it certainly feels like the more correct thing to do.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214631468)
Sounds good! Will work on this now (leaving this reply in case this comment disappears once I rebase)

📝 SamuelMarks opened a pull request: "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration"
(https://github.com/bitcoin/bitcoin/pull/27805)
Note that `size_t` is not always `size_type`, but I haven't replaced your other usages of `size_t` throughout the codebase (I can upon request).

Some details:
- https://en.cppreference.com/w/cpp/container/vector#Member_types
- https://www.ibm.com/docs/en/zos/2.4.0?topic=types-vectorsize-type

Further discussion:
- https://stackoverflow.com/q/17258067
- https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1428r0.pdf a dissent by Bjarne Stroustrup

💬 MarcoFalke commented on pull request "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration":
(https://github.com/bitcoin/bitcoin/pull/27805#issuecomment-1574097015)
We don't support WIN16 or DOS, etc, so your rationale from the stackoverflow link does not apply. The paper you linked to talks about something else. I haven't looked at the other links.

Given that this doesn't change behavior or likely even the resulting binary, this qualifies as a style change:

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the
...

✅ MarcoFalke closed a pull request: "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration"
(https://github.com/bitcoin/bitcoin/pull/27805)

💬 theuni commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#issuecomment-1574102337)
@fanquake and @hebasto have both pointed out to me in discussions that this works even without the `-nostdlibinc`. That is _purely_ accidental and coincidental, as the addition of `-nostdlibinc` is what's _intended_ to make the other changes here work.

By sheer luck though, they manage to put everything in the correct order such that the poisoned paths come last in the search-order. That feels brittle, but maybe good enough.

I'd also like to see what the consequences of the `argument unuse
...

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214670321)
oof. some of the spacing is not quite right here even for cascading. I'm going to fix it in the new compilation unit/file, maybe then it will make a little more sense? I'm worried lining them up isn't going to be much help. It's hard to read too because the declaration for this 4D array happens by nesting arrays in arrays :sob:

📝 Xekyo opened a pull request: "fuzz: Fix mini_miner_selection running out of coin"
(https://github.com/bitcoin/bitcoin/pull/27806)
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior.

Fixed per belt-suspender approach:
- assert that available_coins is not empty before generating tx
- generate at least two coins per
...

💬 sr-gi commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1574166059)
> > What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).
>
> This isn't trading off privacy -- that's the point of [this comment](https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1549711784). It does mean that so
...

💬 achow101 commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574167988)
Turning this on will log private keys as they are written to the db, which seems a little scary.

⚠️ kevkevinpal opened an issue: "depriortisetransaction"
(https://github.com/bitcoin/bitcoin/issues/27807)
### Please describe the feature you'd like to see added.

If a user had already prioritized a transaction to be mined there would be no way to get it deprioritized until after this PR
https://github.com/bitcoin/bitcoin/pull/27501 which adds a new RPC call to getpriotisationmap. There should be a way to remove the prioritization entirely and set it to zero without calling two separate RPCs.



### Is your feature related to a problem, if so please describe it.

If a miner wants to deprioritize
...

💬 pinheadmz commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574169936)
rpc `prioritizetransaction` can be called with a negative delta already, right?

💬 kevkevinpal commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574172151)
> rpc `prioritizetransaction` can be called with a negative delta already, right?

Yea exactly but that requires the user to track the txid and its delta or after https://github.com/bitcoin/bitcoin/pull/27501 is merged track the txid call getpriotisationmap and then use the negative delta from there

was thinking it would be easier if the user could just specify the txid they want to remove

💬 willcl-ark commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574177098)
`getmempoolentry` RPC also shows you the modified fee, which includes the priotirization delta (although still requires reverse calculation).

💬 pinheadmz commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574177955)
> was thinking it would be easier if the user could just specify the txid they want to remove

can't you just call prioritisetransaction with txid and, like, `-999999` ?

💬 kevkevinpal commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574181505)
> > was thinking it would be easier if the user could just specify the txid they want to remove
>
> can't you just call prioritisetransaction with txid and, like, `-999999` ?

wouldn't this make the map delta never include that txid, maybe depriortisetransaction isn't the best naming but instead removing any preference on a txid. Removing it from the mapdeltas. Goal is to set the mapdelta for a txid to 0.

maybe `removepriortisation` instead?

💬 kevkevinpal commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574182072)
> `getmempoolentry` RPC also shows you the modified fee, which includes the priotirization delta (although still requires reverse calculation).

didn't know that thanks, but ya still reverse calculation still required

💬 pinheadmz commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574182839)
> maybe `removepriortisation` instead?

GOTCHA, well maybe instead of a new rpc, maybe `priotirisetransaction <txid> 0` could be a magic word for reset like you want