💬 ishaanam commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235758654)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Why was this added?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235758654)
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Why was this added?
💬 ishaanam commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235753039)
It would be good to have a test for the application of ancestor bump fees with external pre-selected inputs.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1235753039)
It would be good to have a test for the application of ancestor bump fees with external pre-selected inputs.
💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253478816)
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4421
Actually there is still a race condition there. I think the keep_alive is necessary for bitcoind to even acknowledge the blocks only peer exists. We can use debug log or getpeerinfo but seems like either way the socks proxy is closing too soon
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253478816)
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4421
Actually there is still a race condition there. I think the keep_alive is necessary for bitcoind to even acknowledge the blocks only peer exists. We can use debug log or getpeerinfo but seems like either way the socks proxy is closing too soon
💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253502850)
Yep you are absoutley right, I'll remove.
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253502850)
Yep you are absoutley right, I'll remove.
💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253521412)
I added a log message but not sure what else to do. The problem is that `getaddrinfo()` is blocking and `getaddrinfo_a()` segfaults 😭 As long as the DNS request resolves *eventually* the thread will close, even if bitcoind has given up and detached the thread. If `getaddrinfo()` hangs forever then yes you're right that is a leak, but the user also has a serious networking issue.
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1253521412)
I added a log message but not sure what else to do. The problem is that `getaddrinfo()` is blocking and `getaddrinfo_a()` segfaults 😭 As long as the DNS request resolves *eventually* the thread will close, even if bitcoind has given up and detached the thread. If `getaddrinfo()` hangs forever then yes you're right that is a leak, but the user also has a serious networking issue.
💬 Brotcrunsher commented on pull request "DRAFT: Checking for multi/single-value types in UniValue.":
(https://github.com/bitcoin/bitcoin/pull/27994#issuecomment-1622330328)
Closing. Although this might be valid, I currently lack the time to check this in detail. If I find the time to properly check it all, I might reopen it.
(https://github.com/bitcoin/bitcoin/pull/27994#issuecomment-1622330328)
Closing. Although this might be valid, I currently lack the time to check this in detail. If I find the time to properly check it all, I might reopen it.
✅ Brotcrunsher closed a pull request: "DRAFT: Checking for multi/single-value types in UniValue."
(https://github.com/bitcoin/bitcoin/pull/27994)
(https://github.com/bitcoin/bitcoin/pull/27994)
💬 pinheadmz commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1622332067)
> I don't mind the new field, but it's not correct to say users should be discouraged from using any key they import...
@luke-jr Fair point, I updated the OP (no code changes needed)
> legacy wallet support may be dropped in the next couple releases.
@jonatack It may take about that long for this PR to get merged anyway 🤣. So, I can remove the legacy stuff from this PR right now and only descriptor wallet users can enjoy the feature, OR, we can keep `isactive` in there for now and th
...
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1622332067)
> I don't mind the new field, but it's not correct to say users should be discouraged from using any key they import...
@luke-jr Fair point, I updated the OP (no code changes needed)
> legacy wallet support may be dropped in the next couple releases.
@jonatack It may take about that long for this PR to get merged anyway 🤣. So, I can remove the legacy stuff from this PR right now and only descriptor wallet users can enjoy the feature, OR, we can keep `isactive` in there for now and th
...
📝 kibnakamoto opened a pull request: "Gitignore auto-generated Secp256k1 files"
(https://github.com/bitcoin/bitcoin/pull/28032)
After syncing the repo and compiling, three new automatically generated files were not ignored by git.
```
src/secp256k1/src/libsecp256k1-config.h
src/secp256k1/src/libsecp256k1-config.h.in
src/secp256k1/src/stamp-h1
```
I added them to gitignore.
(https://github.com/bitcoin/bitcoin/pull/28032)
After syncing the repo and compiling, three new automatically generated files were not ignored by git.
```
src/secp256k1/src/libsecp256k1-config.h
src/secp256k1/src/libsecp256k1-config.h.in
src/secp256k1/src/stamp-h1
```
I added them to gitignore.
💬 kristapsk commented on pull request "Gitignore auto-generated Secp256k1 files":
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622387611)
I have even some more files after building.
```
Untracked files:
(use "git add <file>..." to include in what will be committed)
nodes_main.txt
seeds_main.txt
src/secp256k1/src/libsecp256k1-config.h
src/secp256k1/src/libsecp256k1-config.h.in
src/secp256k1/src/stamp-h1
src/univalue/build-aux/
src/univalue/test/no_nul
```
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622387611)
I have even some more files after building.
```
Untracked files:
(use "git add <file>..." to include in what will be committed)
nodes_main.txt
seeds_main.txt
src/secp256k1/src/libsecp256k1-config.h
src/secp256k1/src/libsecp256k1-config.h.in
src/secp256k1/src/stamp-h1
src/univalue/build-aux/
src/univalue/test/no_nul
```
💬 sipa commented on pull request "Gitignore auto-generated Secp256k1 files":
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622390337)
These files are not generated anymore; they're probably from an earlier compilation before the secp256k1 subtree was updated to a version that no longer uses the config file.
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622390337)
These files are not generated anymore; they're probably from an earlier compilation before the secp256k1 subtree was updated to a version that no longer uses the config file.
👍 pinheadmz approved a pull request: "test: refactor: deduplicate legacy ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28025#pullrequestreview-1515259415)
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
Reviewed all code. Built and ran tests locally. Nice clean up!
Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in `p2p_segwit.py` so maybe that's more work than its worth.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdF
...
(https://github.com/bitcoin/bitcoin/pull/28025#pullrequestreview-1515259415)
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
Reviewed all code. Built and ran tests locally. Nice clean up!
Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in `p2p_segwit.py` so maybe that's more work than its worth.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdF
...
💬 pinheadmz commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253580835)
Is it worth checking things like the input index exists and sighash is only one byte?
(https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253580835)
Is it worth checking things like the input index exists and sighash is only one byte?
💬 kibnakamoto commented on pull request "Gitignore auto-generated Secp256k1 files":
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622422718)
> These files are not generated anymore; they're probably from an earlier compilation before the secp256k1 subtree was updated to a version that no longer uses the config file.
I used synced and pulled. Then compiled, these files were permenantly there.
As @kristapsk has mentioned, there are even more auto-generated files probably depending on the system one uses.
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622422718)
> These files are not generated anymore; they're probably from an earlier compilation before the secp256k1 subtree was updated to a version that no longer uses the config file.
I used synced and pulled. Then compiled, these files were permenantly there.
As @kristapsk has mentioned, there are even more auto-generated files probably depending on the system one uses.
💬 sipa commented on pull request "Gitignore auto-generated Secp256k1 files":
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622427360)
My guess is that these files are there from an earlier compilation, but weren't reported back then, because they used to be in (secp256k1's) .gitignore file. Now they are reported, but no longer created, because they're no longer in .gitignore.
See https://github.com/bitcoin-core/secp256k1/pull/1178.
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622427360)
My guess is that these files are there from an earlier compilation, but weren't reported back then, because they used to be in (secp256k1's) .gitignore file. Now they are reported, but no longer created, because they're no longer in .gitignore.
See https://github.com/bitcoin-core/secp256k1/pull/1178.
🤔 mzumsande reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1515317576)
ACK 94c9b1f37e335c43c739b853bb9457737b67d73a
I reviewed the code again and did some additional tests with pruning on regtest.
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1515317576)
ACK 94c9b1f37e335c43c739b853bb9457737b67d73a
I reviewed the code again and did some additional tests with pruning on regtest.
💬 hebasto commented on pull request "Gitignore auto-generated Secp256k1 files":
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622466044)
> After syncing the repo and compiling, three new automatically generated files were not ignored by git.
>
> ```
> src/secp256k1/src/libsecp256k1-config.h
> src/secp256k1/src/libsecp256k1-config.h.in
> src/secp256k1/src/stamp-h1
> ```
Since https://github.com/bitcoin/bitcoin/commit/e5c7fcb361d3379c254a52104b4ba25907cd07bb, these files are not generated by the build system.
> I added them to gitignore.
There is no need for this.
(https://github.com/bitcoin/bitcoin/pull/28032#issuecomment-1622466044)
> After syncing the repo and compiling, three new automatically generated files were not ignored by git.
>
> ```
> src/secp256k1/src/libsecp256k1-config.h
> src/secp256k1/src/libsecp256k1-config.h.in
> src/secp256k1/src/stamp-h1
> ```
Since https://github.com/bitcoin/bitcoin/commit/e5c7fcb361d3379c254a52104b4ba25907cd07bb, these files are not generated by the build system.
> I added them to gitignore.
There is no need for this.
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253457842)
eb4dc54 These changes seem needed due to the special member functions added to `CTxMemPoolEntry` in the same commit? If they are changed, a comment in the commit explaining why would be helpful.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253457842)
eb4dc54 These changes seem needed due to the special member functions added to `CTxMemPoolEntry` in the same commit? If they are changed, a comment in the commit explaining why would be helpful.
🤔 jonatack reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1515038131)
Approach ACK with some questions and feedback.
The last push changed from storing the entry time to using the sequence number in `mempool#GetSequence` per https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1230714110.
The `CTxMemPool::m_sequence_number` documentation could use updating in this case regarding its expanded role
```cpp
// In-memory counter for external mempool tracking purposes.
// This number is incremented once every time a transaction
// is added o
...
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1515038131)
Approach ACK with some questions and feedback.
The last push changed from storing the entry time to using the sequence number in `mempool#GetSequence` per https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1230714110.
The `CTxMemPool::m_sequence_number` documentation could use updating in this case regarding its expanded role
```cpp
// In-memory counter for external mempool tracking purposes.
// This number is incremented once every time a transaction
// is added o
...
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253479064)
Unrelated to this pull, but seems odd that `entry_height` is set to 0 here when 1 is the usual initial value.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253479064)
Unrelated to this pull, but seems odd that `entry_height` is set to 0 here when 1 is the usual initial value.
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253438163)
eb4dc54 Are these special member functions [needed](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero), and if yes, [declare them all?](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all) A comment in the commit explaining why they are added would be helpful.
```diff
+ CTxMemPoolEntry() = delete;
+ ~CTxMemPoolEntry() = default;
CTxMemPoolEntry(const CTxMemPoolEnt
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253438163)
eb4dc54 Are these special member functions [needed](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero), and if yes, [declare them all?](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all) A comment in the commit explaining why they are added would be helpful.
```diff
+ CTxMemPoolEntry() = delete;
+ ~CTxMemPoolEntry() = default;
CTxMemPoolEntry(const CTxMemPoolEnt
...