💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956400806)
hmm, above you mentioned "If it doesn't exist in main, it doesn't exist in staging either.".
And now there is a state where the transaction exits in staging but not in main
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956400806)
hmm, above you mentioned "If it doesn't exist in main, it doesn't exist in staging either.".
And now there is a state where the transaction exits in staging but not in main
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956469873)
```suggestion
Assume (level - MAX_LEVELS - 1);
auto& entry = m_entries[idx];
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956469873)
```suggestion
Assume (level - MAX_LEVELS - 1);
auto& entry = m_entries[idx];
```
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956442485)
```suggestion
int level = MAX_LEVELS - 1;
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956442485)
```suggestion
int level = MAX_LEVELS - 1;
```
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956478391)
Should be set to avoid duplicates, will prevent de duplicating later on?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956478391)
Should be set to avoid duplicates, will prevent de duplicating later on?
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956527816)
Hm, I don't want to manually change `-maxorphantxs`. But hey, this is the idea for introducing the test prior to increasing the default `-maxorphantxs`. At that point, the orphanage doesn't go past 100, so it's definitely doing evictions even with just 1010 + 101 orphans.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956527816)
Hm, I don't want to manually change `-maxorphantxs`. But hey, this is the idea for introducing the test prior to increasing the default `-maxorphantxs`. At that point, the orphanage doesn't go past 100, so it's definitely doing evictions even with just 1010 + 101 orphans.
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659951439)
> If these all appear on bitcoincore.org/bin/ then it's starting to look a bit overwhelming for a MacOS (or Windows) user to correctly select what they need.
We used to upload only the binaries, but started to upload all of the guix output because the SHA256SUMS lists all of that. Previously, if you downloaded everything for a release and then tried to verify the SHA256SUMS, it would give you an error about missing files, and we thought that might be confusing.
I think it would be reasonab
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659951439)
> If these all appear on bitcoincore.org/bin/ then it's starting to look a bit overwhelming for a MacOS (or Windows) user to correctly select what they need.
We used to upload only the binaries, but started to upload all of the guix output because the SHA256SUMS lists all of that. Previously, if you downloaded everything for a release and then tried to verify the SHA256SUMS, it would give you an error about missing files, and we thought that might be confusing.
I think it would be reasonab
...
🤔 stickies-v reviewed a pull request: "init: Take lock on blocks directory in BlockManager ctor"
(https://github.com/bitcoin/bitcoin/pull/31860#pullrequestreview-2617967456)
Concept ACK on:
- making LockDirectory more strict, it confused me recently that I was able to acquire a lock multiple times (within the same process)
- making BlockManager manage the lock instead of init
As per https://github.com/TheCharlatan/bitcoin/pull/25, I would be a fan of going further and making the `BlockManagerOptions` ctor take a `BlockDirLock` (or, as per my comment a `DirectoryLock`) to allow more granular error feedback, but this is already a step in the right direction regar
...
(https://github.com/bitcoin/bitcoin/pull/31860#pullrequestreview-2617967456)
Concept ACK on:
- making LockDirectory more strict, it confused me recently that I was able to acquire a lock multiple times (within the same process)
- making BlockManager manage the lock instead of init
As per https://github.com/TheCharlatan/bitcoin/pull/25, I would be a fan of going further and making the `BlockManagerOptions` ctor take a `BlockDirLock` (or, as per my comment a `DirectoryLock`) to allow more granular error feedback, but this is already a step in the right direction regar
...
💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956244596)
It seems like this class could easily be reused for chainstate, datadir, wallet, ... directories in future commits. Perhaps a `class DirLock` would make sense?
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956244596)
It seems like this class could easily be reused for chainstate, datadir, wallet, ... directories in future commits. Perhaps a `class DirLock` would make sense?
💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956484013)
At this point, I don't see the benefit of this function anymore. Let's just inline it?
<details>
<summary>git diff on 436df349f0</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 579669712e..3a984488af 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1107,12 +1107,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
} // no default case, so the compiler can warn about missing cases
assert(false);
}
-static bool LockDirectories(bool probeOn
...
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956484013)
At this point, I don't see the benefit of this function anymore. Let's just inline it?
<details>
<summary>git diff on 436df349f0</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 579669712e..3a984488af 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1107,12 +1107,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
} // no default case, so the compiler can warn about missing cases
assert(false);
}
-static bool LockDirectories(bool probeOn
...
💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956371573)
Copy ctor and assignment operator should probably be deleted?
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956371573)
Copy ctor and assignment operator should probably be deleted?
💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956541082)
And have `BlockManager::m_lock` be a `std::unique_ptr<BlockDirLock>` instead?
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956541082)
And have `BlockManager::m_lock` be a `std::unique_ptr<BlockDirLock>` instead?
📝 brunoerg opened a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870)
This PR splits the `coinselection` fuzz harness into 4 targets (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`, `coinselection`). The goal is to be able to fuzz each algorithm separately (and avoid bad performance) and also all of them together.
(https://github.com/bitcoin/bitcoin/pull/31870)
This PR splits the `coinselection` fuzz harness into 4 targets (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`, `coinselection`). The goal is to be able to fuzz each algorithm separately (and avoid bad performance) and also all of them together.
👋 brunoerg's pull request is ready for review: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870)
(https://github.com/bitcoin/bitcoin/pull/31870)
💬 TheCharlatan commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659997041)
> I'm not sure about the guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz artifact in the output.
I think that has always been there?
Can confirm that the app bundle runs fine, and I am reproducing the hashes. Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659997041)
> I'm not sure about the guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz artifact in the output.
I think that has always been there?
Can confirm that the app bundle runs fine, and I am reproducing the hashes. Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660002068)
> Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
No, these should all be codesigned now.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660002068)
> Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
No, these should all be codesigned now.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956579091)
Ok, I've made it 101 * 30 + 101 = 3131 total. I think the `wait_until` for the 1p1c orphan to be in orphanage kind of achieves what we want (i.e. evictions for each of the previous orphans have already been calculated) even though we don't explicitly wait for each peer.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956579091)
Ok, I've made it 101 * 30 + 101 = 3131 total. I think the `wait_until` for the 1p1c orphan to be in orphanage kind of achieves what we want (i.e. evictions for each of the previous orphans have already been calculated) even though we don't explicitly wait for each peer.
💬 achow101 commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2660041953)
ACK cd4bfaee103591f212b88afd17969c16c1056eb6
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2660041953)
ACK cd4bfaee103591f212b88afd17969c16c1056eb6
📝 Syed-AbdurRahaman2006 opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/31871)
i changed line number 16 from further to for more because of increasing the readability.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin C
...
(https://github.com/bitcoin/bitcoin/pull/31871)
i changed line number 16 from further to for more because of increasing the readability.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin C
...
💬 achow101 commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2660049612)
ACK a759ea3e920dcba52798755ba9f3891f914afbb0
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2660049612)
ACK a759ea3e920dcba52798755ba9f3891f914afbb0