💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1946580499)
addressed all comments
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1946580499)
addressed all comments
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491345962)
Right, not sure what I was thinking here. Will revert and apply your suggestion.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491345962)
Right, not sure what I was thinking here. Will revert and apply your suggestion.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1946629608)
Updated 4e270f3aa70f0bb4e7053d84d5cdd1a1fdcf85c2 -> 75931055b84d1f0e9f44d3d3efc28785fd23e66c ([noGlobalSignals_16](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_16) -> [noGlobalSignals_17](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_16..noGlobalSignals_17))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491319580), reverted previous comm
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1946629608)
Updated 4e270f3aa70f0bb4e7053d84d5cdd1a1fdcf85c2 -> 75931055b84d1f0e9f44d3d3efc28785fd23e66c ([noGlobalSignals_16](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_16) -> [noGlobalSignals_17](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_16..noGlobalSignals_17))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491319580), reverted previous comm
...
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491375756)
Good point, I've changed this to use `WalletStorage` instead. This greatly reduces the diff.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491375756)
Good point, I've changed this to use `WalletStorage` instead. This greatly reduces the diff.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376382)
This was because of using no-op callback, but the change to a `WalletStorage` method obsoletes this.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376382)
This was because of using no-op callback, but the change to a `WalletStorage` method obsoletes this.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376537)
Done
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376537)
Done
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376663)
Added `Assume`s.
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491376663)
Added `Assume`s.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1946691691)
I've also added one more commit that speeds up loading and migration as I noticed one spot where `m_spk_managers` was being iterated again during loading.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1946691691)
I've also added one more commit that speeds up loading and migration as I noticed one spot where `m_spk_managers` was being iterated again during loading.
💬 mzumsande commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491334604)
I don't understand this part of the test. Why do we connect nodes 1 and 0, just to disconnect again without doing anything in between?
Why is the `generate` call necessary? For me, the test also succeeds if I delete it. But if we do have to generate a block, shouldn't we then also at least add a `sync_blocks` to ensure it propagates to all nodes before disconnecting?
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491334604)
I don't understand this part of the test. Why do we connect nodes 1 and 0, just to disconnect again without doing anything in between?
Why is the `generate` call necessary? For me, the test also succeeds if I delete it. But if we do have to generate a block, shouldn't we then also at least add a `sync_blocks` to ensure it propagates to all nodes before disconnecting?
💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1946770281)
@instagibbs
I maintain your review and the ones of @sdaftuar and @achow101 are lacking professionalism.
For a PR which is called "anti-pinning" (it's all in the title), there has been no substantial answer on my concerns that it does not address neither rbf rule3 pinning (the 1000 vbyte rule limit offers to give a way, see "loophole" scenario exposed above), neither topological-based scenario at the tx-relay network level (NTA one), and that actually "hard" limits are a source of "no-one-f
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1946770281)
@instagibbs
I maintain your review and the ones of @sdaftuar and @achow101 are lacking professionalism.
For a PR which is called "anti-pinning" (it's all in the title), there has been no substantial answer on my concerns that it does not address neither rbf rule3 pinning (the 1000 vbyte rule limit offers to give a way, see "loophole" scenario exposed above), neither topological-based scenario at the tx-relay network level (NTA one), and that actually "hard" limits are a source of "no-one-f
...
💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1946779190)
@glozow are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1946779190)
@glozow are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.
💬 ariard commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1946802684)
> One potential solution to this problem is to stop CCing unrelated people on your disclosure emails.
I’m always adding people on embargoed communications for strong technical or operational reasons (e.g timezones) and based on their professional experiences. Embargoed communications not be confused with disclosure emails. If in the future, I”ll apply “no-chaincode / no-spiral” as I can trust their group incentives for now and they show no sign to clarify their pattern of behaviors. Frankly,
...
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1946802684)
> One potential solution to this problem is to stop CCing unrelated people on your disclosure emails.
I’m always adding people on embargoed communications for strong technical or operational reasons (e.g timezones) and based on their professional experiences. Embargoed communications not be confused with disclosure emails. If in the future, I”ll apply “no-chaincode / no-spiral” as I can trust their group incentives for now and they show no sign to clarify their pattern of behaviors. Frankly,
...
💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491464952)
> I don't understand this part of the test. Why do we connect nodes 1 and 2, just to disconnect again without doing anything in between? Why is the generate call necessary?
To ensure all nodes are out of IBD. The idea was to isolate this test case from the previous cases executed along the file. It was done in this way to be able to run this test case at any point of the file. This was vasild request because he was running it first, commenting the rest of the cases to speed the process up.
...
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491464952)
> I don't understand this part of the test. Why do we connect nodes 1 and 2, just to disconnect again without doing anything in between? Why is the generate call necessary?
To ensure all nodes are out of IBD. The idea was to isolate this test case from the previous cases executed along the file. It was done in this way to be able to run this test case at any point of the file. This was vasild request because he was running it first, commenting the rest of the cases to speed the process up.
...
💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491483794)
This is relevant: https://stackoverflow.com/questions/45987571/difference-between-constexpr-and-static-constexpr-global-variable
I made the following experiment:
`s.h`:
```cpp
constexpr const char* s{"aabbcc"};
```
`a.cc`:
```cpp
#include "s.h"
#include <stdio.h>
void a()
{
printf("a(): %p, %s\n", &s, s);
}
```
`b.cc`:
```cpp
#include "s.h"
#include <stdio.h>
void b()
{
printf("b(): %p, %s\n", &s, s);
}
```
`main.cc`:
```cpp
void a();
void b();
int mai
...
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491483794)
This is relevant: https://stackoverflow.com/questions/45987571/difference-between-constexpr-and-static-constexpr-global-variable
I made the following experiment:
`s.h`:
```cpp
constexpr const char* s{"aabbcc"};
```
`a.cc`:
```cpp
#include "s.h"
#include <stdio.h>
void a()
{
printf("a(): %p, %s\n", &s, s);
}
```
`b.cc`:
```cpp
#include "s.h"
#include <stdio.h>
void b()
{
printf("b(): %p, %s\n", &s, s);
}
```
`main.cc`:
```cpp
void a();
void b();
int mai
...
💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491488517)
What do you mean with this last sentence?
Validation events' subscribers will not call this function. This function will be called by the event dispatching side.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491488517)
What do you mean with this last sentence?
Validation events' subscribers will not call this function. This function will be called by the event dispatching side.
💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947241367)
> * The `timingsafe_bcmp` impl in the code was previously always picked. Now it may or may not be picked.
At least for now, this one should be coming from `crypto/common.h`. That wouldn't have been the case after #29263 though.
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947241367)
> * The `timingsafe_bcmp` impl in the code was previously always picked. Now it may or may not be picked.
At least for now, this one should be coming from `crypto/common.h`. That wouldn't have been the case after #29263 though.
💬 theuni commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947251959)
What's the status on this? Just waiting on a change for the crypto defines?
It'd be nice to get it in with/soonafter #29404.
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947251959)
What's the status on this? Just waiting on a change for the crypto defines?
It'd be nice to get it in with/soonafter #29404.
💬 mzumsande commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1491629378)
> I think that is ok, given that pzsDest never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of the node, right?
With the slight exception that DNS seeds can, in certain circumstances, be added as `AddAddrFetch` peers and then queried via `pszDest`. These are not provided by the operator manually but hardcoded.
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1491629378)
> I think that is ok, given that pzsDest never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of the node, right?
With the slight exception that DNS seeds can, in certain circumstances, be added as `AddAddrFetch` peers and then queried via `pszDest`. These are not provided by the operator manually but hardcoded.
💬 mzumsande commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1947291868)
ACK ba021087a2a8d1b5866460a386ea42e0b643c184
I reviewed the code and tested this by manually connecting to a node that resolves to both an IPv4 and an IPv6 address (the latter of which I can't connect to in my network environmen)
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1947291868)
ACK ba021087a2a8d1b5866460a386ea42e0b643c184
I reviewed the code and tested this by manually connecting to a node that resolves to both an IPv4 and an IPv6 address (the latter of which I can't connect to in my network environmen)
🤔 ryanofsky reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1883484751)
Code review c891a65067f57ba87e443d0cdc4f7e6b434c436f.
Thanks for the updates! A lot of nice changes and test coverage here, and I think the PR looks close to ready to ACK, though I did leave a number of suggestions below. Most of these should be very simple or simplify code, so should not create too much work.
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1883484751)
Code review c891a65067f57ba87e443d0cdc4f7e6b434c436f.
Thanks for the updates! A lot of nice changes and test coverage here, and I think the PR looks close to ready to ACK, though I did leave a number of suggestions below. Most of these should be very simple or simplify code, so should not create too much work.