💬 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.
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491449136)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
I was surprised to see this change, since I thought intent of the preceding commit was to preserve current behavior and consider spending transactions active even they conflicted with mempool transactions. Both approaches seem ok, but if this behavior is intended, it seems like it would be simpler to write this condition as:
```c++
if (!wtx.IsAbandoned() && !wtx.isConflicted()) {
return true; // sp
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491449136)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
I was surprised to see this change, since I thought intent of the preceding commit was to preserve current behavior and consider spending transactions active even they conflicted with mempool transactions. Both approaches seem ok, but if this behavior is intended, it seems like it would be simpler to write this condition as:
```c++
if (!wtx.IsAbandoned() && !wtx.isConflicted()) {
return true; // sp
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491442105)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Would suggest expanding comment to something like: "Set of mempool transactions that conflict directly with the tx or an ancestor transaction. This set will be empty if the transaction is confirmed, but can be nonempty in other states."
I was initially surprised to see that this set was populated not just for inactive transactions, but also for block-conflicted transactions, so think it is worth mentioni
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491442105)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Would suggest expanding comment to something like: "Set of mempool transactions that conflict directly with the tx or an ancestor transaction. This set will be empty if the transaction is confirmed, but can be nonempty in other states."
I was initially surprised to see that this set was populated not just for inactive transactions, but also for block-conflicted transactions, so think it is worth mentioni
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491412469)
In commit "scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted" (df0130d73341bb2784b23cd2e8307f30da09ca37)
Not very important, but I think it would be good if this scripted-diff commit renamed `CWalletTx::isConflicted` to `isBlockConflicted` up front, so `isConflicted` could be added as a new method later without affecting existing callers. Otherwise it takes extra effort for reviewers to look up all the existing callers of isConflicted recursively and making sure their behavior
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491412469)
In commit "scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted" (df0130d73341bb2784b23cd2e8307f30da09ca37)
Not very important, but I think it would be good if this scripted-diff commit renamed `CWalletTx::isConflicted` to `isBlockConflicted` up front, so `isConflicted` could be added as a new method later without affecting existing callers. Otherwise it takes extra effort for reviewers to look up all the existing callers of isConflicted recursively and making sure their behavior
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491576720)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It seems wasteful and potentially slow to use RecursiveUpdateTxState in its current form because it will be calling WalletBatch::WriteTx for these transactions and updating the database even though the transaction data will not be changing.
Would suggest fixing this by adding a `CWallet::RecursiveUpdateTxState` overload that takes a `WalletBatch*` parameter and does not call `WriteTx` when it is null. E.
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491576720)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It seems wasteful and potentially slow to use RecursiveUpdateTxState in its current form because it will be calling WalletBatch::WriteTx for these transactions and updating the database even though the transaction data will not be changing.
Would suggest fixing this by adding a `CWallet::RecursiveUpdateTxState` overload that takes a `WalletBatch*` parameter and does not call `WriteTx` when it is null. E.
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491434583)
In commit "wallet: use CWalletTx member functions to determine tx state" (523a48b7180409970cec286f0aef6afbbacd2ffa)
This code comment is just repeating the boolean expression below without explaining it. Would suggest a comment explaining what it is trying to do like "An output is considered spent by a spending transaction unless the spending transaction is conflicted or abandoned. The check below is written conservatively to check for the inverse condition and only consider spending transact
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491434583)
In commit "wallet: use CWalletTx member functions to determine tx state" (523a48b7180409970cec286f0aef6afbbacd2ffa)
This code comment is just repeating the boolean expression below without explaining it. Would suggest a comment explaining what it is trying to do like "An output is considered spent by a spending transaction unless the spending transaction is conflicted or abandoned. The check below is written conservatively to check for the inverse condition and only consider spending transact
...